Switching python unittest to pytest framework

[meimchu]

• This change may also have a small naming requirement as we can see in the pyproject.toml. Also need to use pytest.ini for now to accommodate python2.
• Made a change to a helper function name from test_percent_1000() to helper_test_percent_1000() since pytest config will now discover every functions starting with test_*. However, I thought it may still be worth the switch for possible pytest features we can leverage down the road. Pytest config rules can be set and I have it set to currently:

python_files = "*Test.py"
python_classes = "Test"
python_functions = "test_*"

Just wanted to see what the thought is on switching and the changes in config. May need to change the docs/guides/contributing/unit_tests.rst to reflect this config change as well.

[meimchu]

Mind if I just change the python test files to be test_* so that pytest can automatically pick it up? Looks like in the wheel CI, it downloads the python package which does not include the config file (pytest.ini or pyproject.toml) so it fails to pick up on any tests. To me, it would make sense to just rename the files to follow the default pytest naming convention if you guys agree.

[remia]

Thanks for the PR @meimchu, I don't see issues renaming the tests files if that simplify things. Could you mention some of the benefits of switching to pytest by the way, I'm not opposed to it at all but I think that would be interesting to add in the description of the PR.

Related note for later work, ideally we would update the wheels to stop including the test suite and files in the package and only run the tests with cibuildwheel (as current) pointing to a temporary test directory.

[meimchu]

Thanks @remia for your comment. I think my main motivation for switching over to pytest is that it is probably the most popular python testing framework despite not being part of the python standard library. Currently we use python standard library's unittest which is fine. But I think pytest does have a lot more functionalities that we could potentially take advantage of. I will go ahead and change the test files' naming convention first. Let me know if there is anything specific you want me to do about the cibuildwheel suggestion.

[meimchu]

Hmmm, looks like there is a merge conflict that is preventing the CI/CD from progressing in the main branch here. Unfortunately I don't think I have the the authority to deal with the merge conflict directly on GitHub here.
In my fork'ed branch, the main tests all went through fine but no wheel tests: https://github.com/meimchu/OpenColorIO/actions/runs/3917261040/jobs/6696947907

[remia]

Thanks for the answer @meimchu, it make sense. For the conflict, did you try locally to update your main branch, then go back to your feature branch and do a git rebase main? Then after solving the conflict(s) you can force push here again.

For the cibuildwheel update, I think this could be addressed in a separate PR to keep changes simple, I'll try to have a look when I get time.

[meimchu]

Thanks @remia. I was not quite sure if git push would be allow here due to the rebase. But I did it now after failing to get it to work by just adding in the changes manually. But anyways, I am not sure if I just caught a bad time with the CICD but there are some random jobs that just fail and canceled seemingly unrelated to my changes. For example, I think some look to have automatically cancelled due to the lengthiness of the job? I don't currently think it's the code that is causing this inconsistent CICD behaviour. I do not have the ability to restart those jobs, it seems, so I am not sure what would be the next debug steps. If anybody have pointers, I am all ear. Thanks all!

[remia]

Could be related to #1714 but not 100% sure.

[remia]

Looks like the CI is now fixed!

[doug-walker]

Hi Mei, sorry for the delayed review. We should probably ask at the TSC meeting to confirm that people are ok moving from unittest to pytest.

As you mention, pytest is not part of the standard library. Which means that if people just follow the current instructions for using ctest, none of the Python tests will run (unless they happen to already have pytest on their system). Although we do already have some tests that use numpy, if numpy is missing the rest of the tests are configured so that they still run. With your change, people will now have to do an additional install step if they want to run any Python tests. (Personally, I'm not sure the improvement over unittest justifies adding this step, but I don't object if you think it will be useful.)

Also worth noting is that pytest has dependencies, which means that pip also installs these packages:
tomli, pluggy, packaging, iniconfig, exceptiongroup, attrs

The documentation requirements already bring in Sphinx which brings in a ton of dependent packages too. So I guess adding more at this point is not a big deal, if everyone is ok with it. Although, I'm wondering if we should explore using virtual envs or Poetry to avoid polluting the user's Python install?

At a minimum, please update:
/usr/local/workspace/OpenColorIO-priv/docs/guides/contributing/unit_tests.rst

If someone follows the instructions, the Python tests should run. This means that they need to either run:
share/ci/scripts/<PLATFORM>/install_tests_env.sh
or they need to manually pip install tests/requirements.txt.

The actual changes to the tests seem ok. (In the future, it would be better to avoid all of the unnecessary white space changes, but please don't worry about it for this one.)

[remia]

I agree with @doug-walker comment and hadn't considered the additional implications of running the Python unit tests outside CI.

As discussed in the last TSC, we have some machinery to check for Python packages availability, as seen in the doc CMakeLists.txt find_python_package macro. We should at least check for pytest before enabling the python unit test in the CMake configure step.