TST: Replace pytest.mark.external by enable_socket

[MartinThoma]

pypdf has three types of tests: (1) Tests that work only with the repository itself, (2) tests that require the sample-files git submodule, and (3) tests that require PDF documents from external websites.

As people or projects might not be able to execute (2), they are marked with @pytest.mark.samples

As people or projects might not be able to execute (3), they are marked with @pytest.mark.enable_socket (formerly @pytest.mark.external).

We regularly had issues with people forgetting to mark their tests with external (see #1632).

The pytest-socket pytest plugin disables the usage of sockets on the whole project, except for the tests marked with enable_socket. This was, we can guarantee that the CI will catch missing @pytest.mark.enable_socket.

[MartinThoma]

@dkg Please let me know what you think :-)

Sadly, this means that people using the external flag at the moment need to replace it.

Do you think we should have both flags for a while?

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (ae42106) 92.47% compared to head (9a92904) 92.47%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1657   +/-   ##
=======================================
  Coverage   92.47%   92.47%           
=======================================
  Files          33       33           
  Lines        6495     6495           
  Branches     1287     1287           
=======================================
  Hits         6006     6006           
  Misses        315      315           
  Partials      174      174           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dkg]

I don't think i fully understand how this change works to enable your CI to catch missing/broken annotations, but i also don't mind updating the debian packaging once this is merged to change any testing configuration that sets aside external tests to make it set aside enable_socket tests. that's a pretty straightforward change, and well worth it, if the result is improved upstream attention to which tests need network access.

[MartinThoma]

The pytest plugin disabled network connections, except when this marker is set. Meaning CI would fail if the marker is missing, because the test cannot download the file. Hence this guarantees the we never forget to at the marker. But we might forget to remove it, if we get rid of the external file

[MasterOdin]

To add a bit more detail to what @MartinThoma said since I was curious how this worked, this PR adds the pytest-socket plugin to pytest, where by passing the --disable-socket flag to pytest.addopts, then the plugin will block all requests over a socket (be it HTTP or otherwise) will throw an error, unless you use the pytest.mark.enable_socket marker, which is a special to the aforementioned plugin and it turns back on sockets.

For any test that doesn't have that marker, we should see a SocketBlockedError get thrown, as we do right now in the CI with the last commit since I guess a test doesn't have it properly marked, which shows the value of this change.

But we might forget to remove it, if we get rid of the external file

I think this is definitely better than the inverse for projects like debian that want to run a subset of tests, and where this repo will always run all tests anyway.

[MartinThoma]

@dkg If it's helpful to you, I could make a release at a fixed time so that you can adjust the ignored pytest mark from external to enable_socket :-)

[MartinThoma]

@dkg The first version with that fix is now released. Meaning you will not have to worry about this issue again :-)