Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failure of Black Test in CI testing in github #119

Open
nadamoukaddem opened this issue Feb 17, 2024 · 10 comments · Fixed by #120
Open

Failure of Black Test in CI testing in github #119

nadamoukaddem opened this issue Feb 17, 2024 · 10 comments · Fixed by #120
Labels
bug Something isn't working

Comments

@nadamoukaddem
Copy link
Contributor

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
Copy link
Contributor Author

nadamoukaddem commented Feb 21, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@jeipollack
Copy link
Contributor

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

@nadamoukaddem
Copy link
Contributor Author

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
Copy link
Contributor

jeipollack commented Feb 22, 2024

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

@nadamoukaddem nadamoukaddem linked a pull request Feb 22, 2024 that will close this issue
@jeipollack
Copy link
Contributor

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

  • Remove pytest-black and --black from the pyproject.toml file
  • Add a run a step to run black during CI like you did previously (ci.yaml)
  • Add a checkbox in the Pull Request template for the reviewer to check the CI log to evaluate whether any files had to be changed or not

Let me know if something is unclear.

@nadamoukaddem
Copy link
Contributor Author

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

@jeipollack
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants