openpreserve / jpylyzer

JP2 (JPEG 2000 Part 1) validator and properties extractor. Jpylyzer was specifically created to check that a JP2 file really conforms to the format's specifications. Additionally jpylyzer is able to extract technical characteristics.
http://jpylyzer.openpreservation.org/
Other
69 stars 28 forks source link

Hard-coded path in test_test.py (PY27 support and Travis questions) #189

Closed ross-spencer closed 1 year ago

ross-spencer commented 1 year ago

I was looking at the Python 2.7 issues following the 2.1 announcement. These are stopping Travis from passing.

Some issues are easier than others, e.g. the encoding string missing in test_testfiles.py:

diff --git a/tests/unit/test_testfiles.py b/tests/unit/test_testfiles.py
index 1414027..e130227 100644
--- a/tests/unit/test_testfiles.py
+++ b/tests/unit/test_testfiles.py
@@ -1,4 +1,5 @@
 #! /usr/bin/env python3
+# -*- coding:utf-8 -*-
 # pylint: disable=missing-docstring
 """
 Tests on jpylyzer test corpus files.

Others are a bit trickier. I tried looking at test_test.py, but we have a couple of different issues:

https://github.com/openpreserve/jpylyzer/blob/92d3eaa409733988e09700d79be3803f58028794/tests/unit/test_test.py#L44

Requires path.walk() or perhaps import of pathlib2 for pathlib.iterdir() would work well for py2 and py3.

But we have this hard-coded path too: https://github.com/openpreserve/jpylyzer/blob/92d3eaa409733988e09700d79be3803f58028794/tests/unit/test_test.py#L41

From the module docstring: https://github.com/openpreserve/jpylyzer/blob/92d3eaa409733988e09700d79be3803f58028794/tests/unit/test_test.py#L8

it seems this maybe should be corrected to ../jpylyzer-test-files but it's not clear and different test errors start to appear once this is corrected.

NB. It seems to work because pytest.parametize() likely receives an empty array [] which satisfies the requirement to be iterable. It means nothing runs on my environment (test is skipped) but it will work on Johan's.

In one draft of my updates I've suggested a check to make sure it is there before we arrive in the test (this just makes sense when trying not to refactor too much):assert os.path.isdir(testFilesDir), "jpylyzer test files directory not found: {}".replace(testFilesDir)

It's not clear the intent of test_xml_onefile(). It looks like an integration test and it catches anything that may fall out of the code that may be unknown, i.e. unhandled exceptions from odd files. Which makes sense.

My questions though are two fold:

  1. Does the test file test_test.py warrant inclusion in the code-base still? What needs to be done for it to be run by maintainers? Do we have access to all the test files required in jpylyzer-test-files
  2. Does Python 2.7 compatibility make sense still, should it just be removed from travis from now? If it does make sense, what steps need to be taken to bring it up to 2.7 before it is removed in 2.2?

Connected to https://github.com/openpreserve/jpylyzer/issues/179

bitsgalore commented 1 year ago

Thanks reporting this @ross-spencer! As for the test_test.py file: I should have deleted that earlier, it was just something I created locally to figure out how to implement the tests that ended up in test_testfiles.py. Done now!

Your comments also made me realise that Travis CI is still working, but that the web domain of the server has changed. I just updated that as well in the readme file. After that I still ended up with Travis failures. For the Python 3.5 build I managed to fix all of these but one, see the separate issue I created for details: https://github.com/openpreserve/jpylyzer/issues/190.

About the Python 2.7 support and Travis: good question . For now I just added the UTF-8 encoding declarations. This does get rid of the error you reported, but then further down the line other errors crop up, with a lot of them looking like Python 3/Python 2 compatibility issues (e.g. see here.

Personally I'm extremely reluctant to dedicate any time fixing this at this stage, as Py 2.7 support in Jpylyzer is approaching its end anyway.

bitsgalore commented 1 year ago

Py 3.5 Travis build works now https://github.com/openpreserve/jpylyzer/issues/190

ross-spencer commented 1 year ago

Nice work Johan, and thanks for the feedback also. All makes sense. I would also take the same approach with 2.7. Largely I was just wondering what the intention between the 2.1 release and 2.2 release may be with regard to fixing anything. All makes sense! :slightly_smiling_face: