Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: Stabilize Pillow test with Pillow missing #2229

Merged
merged 4 commits into from
Oct 1, 2023

Conversation

stefan6419846
Copy link
Collaborator

There have been two issues with the existing test:

  • The name was incorrect, as it never really checked ImageMagick stuff.
  • Running from pypdf import _xobj_image_helpers beforehand, for example because of other tests, would lead to "strange" failures due to Python not re-importing modules which already have been imported.

@stefan6419846
Copy link
Collaborator Author

I ignored UP022 for now and we should probably ignore this on the root level while Python 3.6 is still supported as capture_output is available from Python 3.7 only (https://docs.python.org/3/library/subprocess.html#subprocess.run):

UP022: Sending stdout and stderr to PIPE is deprecated, use capture_output

@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (41ffc2c) 94.38% compared to head (b8b72e8) 94.36%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2229      +/-   ##
==========================================
- Coverage   94.38%   94.36%   -0.03%     
==========================================
  Files          43       43              
  Lines        7591     7591              
  Branches     1498     1498              
==========================================
- Hits         7165     7163       -2     
- Misses        262      264       +2     
  Partials      164      164              

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stefan6419846
Copy link
Collaborator Author

The negative side effect of this change is that it will show the corresponding section as not covered by the tests anymore. After some failed attempts within other projects in the past, I tried to get https://coverage.readthedocs.io/en/stable/subprocess.html to work for this case, but the offending ImportError would stay uncovered. If someone wants to have a look at this, feel free to do so.

@MartinThoma MartinThoma merged commit 30db3ef into py-pdf:main Oct 1, 2023
13 of 14 checks passed
@MartinThoma
Copy link
Member

Thank you 🤗 And thank you for the explanations 🙏

@MartinThoma
Copy link
Member

A comment by @stefan6419846 which was in #2228 :

I assume that the failing test is a side-effect of the newly introduced test: test_image_without_imagemagic seems to implicitly rely on the fact that pypdf._xobj_image_helpers has not been loaded before. The new test retrieves an image, thus probably already importing it, while Python does not re-import already known namespaces. For this reason, the ImportError will not be raised here.

One possible solution to circumvent this is by making test_image_without_imagemagic completely standalone, for example by doing a subprocess call which checks for the ImportError.

A rough draft:

@pytest.mark.enable_socket()
def test_image_without_pillow(tmp_path):
    url = "https://corpora.tika.apache.org/base/docs/govdocs1/914/914102.pdf"
    name = "tika-914102.pdf"
    _ = get_data_from_url(url, name=name)
    path = Path(__file__).parent / "pdf_cache" / name

    source_file = tmp_path / "script.py"
    source_file.write_text(f"""
import sys
from io import BytesIO
from pypdf import PdfReader

import pytest


sys.modules["PIL"] = None
reader = PdfReader("{path.resolve()}", strict=True)

for page in reader.pages:
    with pytest.raises(ImportError) as exc:
        page.images[0]
    assert exc.value.args[0] == (
        "pillow is required to do image extraction. "
        "It can be installed via 'pip install pypdf[image]'"
    ), exc.value.args[0]
""")
    result = subprocess.run([shutil.which("python"), source_file], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    assert result.returncode == 0
    assert result.stdout == b""
    assert result.stderr == b"Superfluous whitespace found in object header b'4' b'0'\n"

@stefan6419846 stefan6419846 deleted the stabilize-test branch October 1, 2023 08:56
MartinThoma added a commit that referenced this pull request Oct 8, 2023
## What's new

### Bug Fixes (BUG)
-  invalid cm/tm in visitor functions (#2206) by @pubpub-zz
-  Encrypt / decrypt Stream object dictionaries (#2228) by @pilotandy
-  Support nested color spaces for the /DeviceN color space (#2241) by @Stefan
-  images property fails if NullObject in list (#2215) by @pubpub-zz

### Robustness (ROB)
-  XYZ destination to cope with missing left and top param (#2237) by @pubpub-zz

### Documentation (DOC)
-  Add pilotandy for #2228 as a contributor by @MartinThoma
-  Link to CONTRIBUTING.md in README.md (#2232) by @markpfeifle
-  Changelog by @MartinThoma

### Developer Experience (DEV)
-  Exclude tests from mypy checks by @MartinThoma
-  Unify mypy options and warn redundant workarounds (#2223) by @exiledkingcc
-  Stabilize Pillow test with Pillow missing (#2229) by @Stefan

### Maintenance (MAINT)
-  Update pinned packages (#2243) by @MartinThoma

### Testing (TST)
-  Regression test against non-deterministic accidental object reuse (#2244) by @MartinThoma

[Full Changelog](3.16.2...3.16.3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants