Clarification and improvements in test_filters.py

[j-t-1]

pypdf/tests/test_filters.py

Lines 45 to 53 in 56cf557

	filter_inputs = (
	string.ascii_lowercase,
	string.ascii_uppercase,
	string.ascii_letters,
	string.digits,
	string.hexdigits,
	string.punctuation,
	string.whitespace, # Add more...
	)

Any ideas on what to add so we can remove the comment?

pypdf/tests/test_filters.py

Lines 71 to 87 in 56cf557

	def test_flatedecode_unsupported_predictor():
	"""
	FlateDecode raises PdfReadError for unsupported predictors.
	Predictors outside the [10, 15] range are not supported.
	This test function checks that a PdfReadError is raised when decoding with
	unsupported predictors. Once this predictor support is updated in the
	future, this test case may be removed.
	"""
	codec = FlateDecode()
	predictors = (-10, -1, 0, 9, 16, 20, 100)
	for predictor, s in cartesian_product(predictors, filter_inputs):
	s = s.encode()
	with pytest.raises(PdfReadError):
	codec.decode(codec.encode(s), DictionaryObject({"/Predictor": predictor}))

We support all the predictor values (1-2, 10-15). I think we should keep the test for values outside the range. So the comment needs updating.

pypdf/tests/test_filters.py

Lines 136 to 140 in 56cf557

	def test_ascii_hex_decode_missing_eod():
	"""ASCIIHexDecode.decode() raises error when no EOD character is present."""
	# with pytest.raises(PdfStreamError) as exc:
	ASCIIHexDecode.decode("")
	# assert exc.value.args[0] == "Unexpected EOD in ASCIIHexDecode"

The docstring does not match the test, so the test needs changing?

pypdf/tests/test_filters.py

Lines 236 to 252 in 56cf557

	def test_ccitt_fax_decode():
	data = b""
	parameters = DictionaryObject(
	{"/K": NumberObject(-1), "/Columns": NumberObject(17)}
	)
	# This was just the result pypdf 1.27.9 returned.
	# It would be awesome if we could check if that is actually correct.
	assert CCITTFaxDecode.decode(data, parameters) == (
	b"II*\x00\x08\x00\x00\x00\x08\x00\x00\x01\x04\x00\x01\x00\x00\x00\x11\x00"
	b"\x00\x00\x01\x01\x04\x00\x01\x00\x00\x00\x00\x00\x00\x00\x02\x01"
	b"\x03\x00\x01\x00\x00\x00\x01\x00\x00\x00\x03\x01\x03\x00\x01\x00"
	b"\x00\x00\x04\x00\x00\x00\x06\x01\x03\x00\x01\x00\x00\x00\x00\x00"
	b"\x00\x00\x11\x01\x04\x00\x01\x00\x00\x00l\x00\x00\x00\x16\x01"
	b"\x04\x00\x01\x00\x00\x00\x00\x00\x00\x00\x17\x01\x04\x00\x01\x00"
	b"\x00\x00\x00\x00\x00\x00\x00\x00"
	)

Needs checking; is the b"x00l" wrong?

[stefan6419846]

Any ideas on what to add so we can remove the comment?

Any suitable test string suitable for round-trip testing. I do not think that removing the comment has any benefit.

We support all the predictor values (1-2, 10-15). I think we should keep the test for values outside the range. So the comment needs updating.

It might be the case if this is backed by a link to the source when updating it.

The docstring does not match the test, so the test needs changing?

Both need changing. 42f970e changed this to warnings, thus we should make sure in the test that it does indeed emit warnings.

Needs checking; is the b"x00l" wrong?

What do you want to check? Why do you consider the sequence \x00l wrong?

[j-t-1]

What do you want to check?

assert CCITTFaxDecode.decode(data, parameters) == ( 
         b"II*\x00\x08\x00\x00\x00\x08\x00\x00\x01\x04\x00\x01\x00\x00\x00\x11\x00" 
         b"\x00\x00\x01\x01\x04\x00\x01\x00\x00\x00\x00\x00\x00\x00\x02\x01" 
         b"\x03\x00\x01\x00\x00\x00\x01\x00\x00\x00\x03\x01\x03\x00\x01\x00" 
         b"\x00\x00\x04\x00\x00\x00\x06\x01\x03\x00\x01\x00\x00\x00\x00\x00" 
         b"\x00\x00\x11\x01\x04\x00\x01\x00\x00\x00l\x00\x00\x00\x16\x01" 
         b"\x04\x00\x01\x00\x00\x00\x00\x00\x00\x00\x17\x01\x04\x00\x01\x00" 
         b"\x00\x00\x00\x00\x00\x00\x00\x00" 
     )

Why do you consider the sequence \x00l wrong?

The two bytes should be split in two for consistency with the other bytes.

[stefan6419846]

I still do not understand what you expect: The assertion is there and running. What additional checks do you want to do?

The two bytes should be split in two for consistency with the other bytes.

I unfortunately do not get this as well: These are two bytes already. l and II* are bytes/byte sequences which can be displayed properly, while all other byte values are hexadecimal due to representing non-printable ASCII characters. For me, the string itself and its inclusion in the test file is completely fine.

[j-t-1]

I have not explained this part well. For the assert we have the data, but we need to check the data since the data were generated by pypdf? So we are self-checking. If this is not correct the code comment needs removing.

For the second part, thanks to your comment I understand this now.

[stefan6419846]

As far as I can see, there is not much to check here - we basically decode an empty image. The expected value is just the header of the corresponding TIFF image, which cannot be opened with regular image viewers. I guess we can just replace the comment then as our TIFF headers are working for proper inputs.

[j-t-1]

@stefan6419846 thanks for feedback. I will do a PR.

Both need changing. 42f970e changed this to warnings, thus we should make sure in the test that it does indeed emit warnings.

What is a good example of checking warnings?

[stefan6419846]

For warnings, you might want to have a look at test_lzw_decoder_table_overflow.