CosmoStat / wf-psf

Data-driven wavefront-based PSF modelling framework.
MIT License
19 stars 9 forks source link

Failure of Black Test in CI testing in github #119

Open nadamoukaddem opened 9 months ago

nadamoukaddem commented 9 months ago

Description of the bug: I opened a pull request to add some documentation files. However, during CI testing on GitHub, some .py files failed the black test (see image below and this link for the full log description), despite passing when I ran these tests on my Linux machine. Note that I didn't make any changes to these Python files in the PR.

pytest

Expected behavior: All files should pass the CI testing.

nadamoukaddem commented 9 months ago

There seems to be a difference between the version of Black installed locally (23.12.1) and the version used in the CI environment (24.2.0), despite both using the same command pip install ".[test]"

jeipollack commented 9 months ago

I don't understand your point. The new version of black==24.2.0 should be different than black==23.12.1, else there's nothing to release. But, yes, for black==24.2.0 the files which passed with the previous version of black==23.12.0 are no longer compliant. So, the solution is to upgrade black and rerun on it on all files in the repo with black . to reformat those files which require it.

nadamoukaddem commented 9 months ago

I added some code to the ci.yml file, and I thought it would automatically perform the reformatting. maybe I made a mistake.

jeipollack commented 9 months ago

Did you verify that the files were reformatted? How would you see these changes in the Pull Request?

nadamoukaddem commented 9 months ago

When I checked the log of the Black formatter, it is written "36 files would be left unchanged", but these files were already passing the CI test. However, it didn't provide any information about the other 8 files that failed the test. so, I assumed that nothing has changed. I will see if I can run the Black formatter before the Black testing in ci.yml file. If not, I will apply the black . command to all files.

jeipollack commented 9 months ago

The CI log doesn't mean that files in the repo (associated to the PR) have changed. Remember these files are not considered to be formatted correctly with respect to the latest version of Black, and we want them to be correct.

It's good to review how CI works in Github Actions: https://docs.github.com/en/actions/automating-builds-and-tests/about-continuous-integration

jeipollack commented 9 months ago

From today's meeting the way forward in resolving this issue is to do the following:

Let me know if something is unclear.

nadamoukaddem commented 9 months ago

Done. I also removed pytest-cases to avoid encountering this error during pytest test: pytest-cases

jeipollack commented 9 months ago

I am not sure whether the solution to remove pytest-cases is the correct one. Could you please open a new ticket to track this new problem?

jeipollack commented 9 months ago

Actually, I checked and I'm not using pytest-cases to set up test cases, rather I am using pytest.mark.parameterize, so it's okay to remove it. But, it's odd that removing it resolves this conflict.