Skip to content

Corrected check for libtiff feature - #7975

Merged
radarhere merged 4 commits into
python-pillow:mainfrom
radarhere:libtiff
Apr 21, 2024
Merged

Corrected check for libtiff feature#7975
radarhere merged 4 commits into
python-pillow:mainfrom
radarhere:libtiff

Conversation

@radarhere

@radarhere radarhere commented Apr 13, 2024

Copy link
Copy Markdown
Member

def test_ifd_rational_save(tmp_path: Path) -> None:
methods = [True]
if features.check("libtiff"):
methods.append(False)
for libtiff in methods:
TiffImagePlugin.WRITE_LIBTIFF = libtiff

This is the wrong way around. WRITE_LIBTIFF should be True when libtiff is available.

To demonstrate, if I skip building libtiff on Windows, it fails. With this change, it passes.

@nulano

nulano commented Apr 13, 2024

Copy link
Copy Markdown
Contributor

Should we use pytest.parameterize with a skip mark to make it clear that the WRITE_LIBTIFF = True test was skipped in the logs?

@radarhere

Copy link
Copy Markdown
Member Author

Ok, I've updated the commit.

@nulano nulano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to use monkeypatch in case the save method raises an exception?

Comment thread Tests/test_tiff_ifdrational.py Outdated
@pytest.mark.parametrize(
"libtiff", (pytest.param(True, marks=skip_unless_feature("libtiff")), False)
)
def test_ifd_rational_save(tmp_path: Path, libtiff: bool) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_ifd_rational_save(tmp_path: Path, libtiff: bool) -> None:
def test_ifd_rational_save(monkeypatch: pytest.MonkeyPatch, tmp_path: Path, libtiff: bool) -> None:

Comment thread Tests/test_tiff_ifdrational.py Outdated
out = str(tmp_path / "temp.tiff")
res = IFDRational(301, 1)
im.save(out, dpi=(res, res), compression="raw")
TiffImagePlugin.WRITE_LIBTIFF = libtiff

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TiffImagePlugin.WRITE_LIBTIFF = libtiff
monkeypatch.setattr(TiffImagePlugin, "WRITE_LIBTIFF", libtiff)

Comment thread Tests/test_tiff_ifdrational.py Outdated
im.save(out, dpi=(res, res), compression="raw")
TiffImagePlugin.WRITE_LIBTIFF = libtiff
im.save(out, dpi=(res, res), compression="raw")
TiffImagePlugin.WRITE_LIBTIFF = False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TiffImagePlugin.WRITE_LIBTIFF = False

@radarhere

Copy link
Copy Markdown
Member Author

I've pushed a commit to use monkeypatch whenever the tests set READ_LIBTIFF or WRITE_LIBTIFF

Comment thread Tests/test_file_libtiff.py Outdated
libtiffs = []
libtiffs = [False]
if Image.core.libtiff_support_custom_tags:
libtiffs.append(True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also use pytest.mark.parametrize

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@nulano nulano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; I just noticed one more test that can be parametrized.

Comment thread Tests/test_imagesequence.py Outdated
TiffImagePlugin.READ_LIBTIFF = True
def test_libtiff(monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.setattr(TiffImagePlugin, "READ_LIBTIFF", True)
_test_multipage_tiff()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While not as useful, these two tests can also be combined into one with pytest.mark.parametrize for slightly shorter code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done

@radarhere
radarhere merged commit eee633c into python-pillow:main Apr 21, 2024
@radarhere
radarhere deleted the libtiff branch April 21, 2024 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants