Skip to content

Conversation

@bouweandela
Copy link
Member

@bouweandela bouweandela commented Mar 4, 2021

The flake8 tests were no longer running on CircleCI and it looks like this happened because we were testing the installed copy of the tool rather than the source code. This fixes the problem by running the flake8 tests before removing the source code for all other unit tests.

Fixes #2049

@valeriupredoi
Copy link
Contributor

OK @bouweandela ready to merge? 🤣

@bouweandela bouweandela changed the title Test that a flake8 issue is found Fix flake8 tests on CircleCI Mar 10, 2021
@valeriupredoi
Copy link
Contributor

valeriupredoi commented Mar 10, 2021

cheers @bouweandela - so we end up running the tests twice? can we not put the flake8 test into a test module and just run that? something like this

@bouweandela
Copy link
Member Author

That's not quite what this does: it first only runs the flake8 tests, then removes the source code because we want to test the installed copy, and then runs only the other tests, so all tests are run exactly once.

@bouweandela bouweandela marked this pull request as ready for review March 10, 2021 19:57
@valeriupredoi
Copy link
Contributor

one second, am running this on me machine

@bouweandela
Copy link
Member Author

You can see that it worked in the one but last commit, there the flake8 issue is found: https://app.circleci.com/pipelines/github/ESMValGroup/ESMValTool/4496/workflows/6a98d906-79dd-4bed-85ed-1c1f5156ac37/jobs/43648

@valeriupredoi
Copy link
Contributor

OK, how is this working - running pytest -m flake8 on my machine it fails (as it should be coz there is an extra space):

_____________________________________________________ FLAKE8-check ______________________________________________________
[gw1] linux -- Python 3.9.2 /home/valeriu/miniconda3py39/envs/esmvaltool/bin/python3.9
/home/valeriu/ESMValCore/esmvalcore/_main.py:35:36: E202 whitespace before ')'

but the fail is not picked up on the CI tests below

@valeriupredoi
Copy link
Contributor

You can see that it worked in the one but last commit, there the flake8 issue is found: https://app.circleci.com/pipelines/github/ESMValGroup/ESMValTool/4496/workflows/6a98d906-79dd-4bed-85ed-1c1f5156ac37/jobs/43648

yeah was just writing my comment when you posted yours - but why is it not picking it up always?

@bouweandela
Copy link
Member Author

Because I fixed the flake8 error in the last commit, so we can get this merged

@valeriupredoi
Copy link
Contributor

no you didn't - it's still there in the _main.py

@bouweandela
Copy link
Member Author

Which _main.py? There is no _main.py in the ESMValTool repository?

@valeriupredoi
Copy link
Contributor

oh c**p I was in the wrong package all this time 🤦‍♂️

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should plop the same test in ESMValCore - I already found an issue there 🤣

@valeriupredoi
Copy link
Contributor

the fact that there is a branch with the same name https://github.com/ESMValGroup/ESMValCore/tree/test-flake8 confused the flake out of me. I am going to plop the same CI test on that branch man

@bouweandela
Copy link
Member Author

That's surprising because there we do not remove the source code before running the tests. Maybe we should though, because that way you're sure you're actually testing that the package works if it's installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run basic Python tests on CI everytime in a PR with target master

3 participants