Corrected check for libtiff feature by radarhere · Pull Request #7975 · python-pillow/Pillow

Corrected check for libtiff feature by radarhere · Pull Request #7975 · python-pillow/Pillow

Conversation

	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.

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

Ok, I've updated the commit.

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?

		@pytest.mark.parametrize(
		"libtiff", (pytest.param(True, marks=skip_unless_feature("libtiff")), False)
		)
		def test_ifd_rational_save(tmp_path: Path, libtiff: bool) -> None:

Choose a reason for hiding this comment

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

	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:

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

Choose a reason for hiding this comment

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

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

		im.save(out, dpi=(res, res), compression="raw")
		TiffImagePlugin.WRITE_LIBTIFF = libtiff
		im.save(out, dpi=(res, res), compression="raw")
		TiffImagePlugin.WRITE_LIBTIFF = False

Choose a reason for hiding this comment

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

TiffImagePlugin.WRITE_LIBTIFF = False

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

		libtiffs = []
		libtiffs = [False]
		if Image.core.libtiff_support_custom_tags:
		libtiffs.append(True)

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

Choose a reason for hiding this comment

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

Done

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.

		TiffImagePlugin.READ_LIBTIFF = True
		def test_libtiff(monkeypatch: pytest.MonkeyPatch) -> None:
		monkeypatch.setattr(TiffImagePlugin, "READ_LIBTIFF", True)
		_test_multipage_tiff()

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.

Choose a reason for hiding this comment

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

Ok, done