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

Fix test_naming.py so it doesn't let through directories that need be ignored #3082

Merged
merged 13 commits into from
Mar 10, 2023

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Mar 8, 2023

Description

For some reason pytest is looking into the .git dir but only for Pythons 3.8 and 3.9 - this is new behaviour, see failed test which is reproducible. Am very confused as to why this is happening!


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks @valeriupredoi! It would be good to have this in soon so that we have ✅ tests to merge other PRs into main 👍

@valeriupredoi
Copy link
Contributor Author

cheers @remi-kazeroni - @schlunma or @bouweandela one of you guys could have a look and merge please 🍺

@bouweandela
Copy link
Member

For some reason pytest is looking into the .git dir but only for Pythons 3.8 and 3.9 - this is new behaviour, see failed test which is reproducible. Am very confused as to why this is happening!

How about opening an issue with pytest and asking about it?

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

I don't think this fixes the actual problem. If I read the doc of the ignore option correctly,

--ignore=path         ignore path during collection (multi-allowed).
--ignore-glob=path    ignore path pattern during collection (multi-allowed).

then this just ignores the .git directory when looking for tests, but does not ignore it when running the test.

I guess we have to change the actual test

def test_avoid_casing_collisions(self):
"""
Check that there are no names differing only in the capitalization
This includes folders differing from files
"""
for dirpath, dirnames, filenames in os.walk(self.esmvaltool_folder):
for dirname in dirnames:
if dirname in IGNORE:
dirnames.remove(dirname)
self.assertEqual(
len(filenames) + len(dirnames),
len({name.lower()
for name in filenames + dirnames}),
'Colliding names found at {0}. Please do not '
'use names that only differ in '
'capitalization'.format(dirpath))

to ignore the entire .git directory and all it's subdirs, and not only the .git itself (which is currently the case).

@valeriupredoi
Copy link
Contributor Author

@bouweandela that is indeed a very good point! I'll do that now - but in the meantime, let's get this in so we have our tests pass, we can always revert once pytest find a solution/release a new version 🍺

@valeriupredoi
Copy link
Contributor Author

@schlunma I don't think there is a need to change the test - this fix does the trick since I ran the GA tests

@schlunma
Copy link
Contributor

I don't think this an issue of pytest...this is our test failing because there are two objects in the .git dir with the same name but different capitalization. The fact that this is happening only for Python 3.8 and 3.9 in that particular case is probably random. I am not sure how git creates subdirs and files in .git, but there is probably some random element.

@schlunma
Copy link
Contributor

As you can see, now the 3.10 test is also failing: https://github.com/ESMValGroup/ESMValTool/actions/runs/4382991244/jobs/7672693535

@valeriupredoi
Copy link
Contributor Author

it isn't random, but it is reproducible (ran a few instances since I too thought that was a fluke) - why? I don't know TBF

@valeriupredoi
Copy link
Contributor Author

As you can see, now the 3.10 test is also failing: https://github.com/ESMValGroup/ESMValTool/actions/runs/4382991244/jobs/7672693535

oh that's a spanner in the works - OK so it's not Py version-related then 👍 Let me have a closer look at the test then

@valeriupredoi
Copy link
Contributor Author

here's a fun one: Py3.8 passing here, when most of the times it fails - so it's not 100% reproducible either https://github.com/ESMValGroup/ESMValTool/actions/runs/4369963301/jobs/7644357766

@schlunma
Copy link
Contributor

Yeah, I guess git is somehow (more or less random) producing files/dirs that cause our tests to fail. I think ignoring the entire .git dir with all it's subdirectories in our test should be the way to go 👍

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Mar 10, 2023

@schlunma you were spot-on, mate! The problem was with our test, and not pytest at all - we had a fairly poor implementation and ignored dirs were slipping through, in my case locally .github kept passing through - I fixed it good now, Maginot Line 😆

@valeriupredoi valeriupredoi changed the title Pytest ignore .git directory Fix test_naming.py so it doesn't let through directories that need be ignored Mar 10, 2023
@valeriupredoi
Copy link
Contributor Author

@schlunma @bouweandela @remi-kazeroni proves out this test was really not working properly as it was before, two-fold:

  • it was looking into top-level dirs that should be ignored
  • it was using dir content from dirs that were ignored at top-level, but content was still walked on and used

It is now fixed and does what it says on the tin, please re-review when you have a bit of time. Cheers to Manu for pulling the alarm lever! 🍺

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Thanks @valeriupredoi, this looks much better! A couple of minor comments.

.github/workflows/test.yml Outdated Show resolved Hide resolved
tests/unit/test_naming.py Outdated Show resolved Hide resolved
tests/unit/test_naming.py Outdated Show resolved Hide resolved
valeriupredoi and others added 3 commits March 10, 2023 15:20
Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>
Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Great, thanks V!! 🍻

@remi-kazeroni
Copy link
Contributor

Thanks a lot @valeriupredoi and @schlunma! Let's get this in to get our tests fixed and resume merging other PRs 🍺

@remi-kazeroni remi-kazeroni merged commit 0cf7e54 into main Mar 10, 2023
@remi-kazeroni remi-kazeroni deleted the pytest_ignore_dotgit_dir branch March 10, 2023 16:18
@valeriupredoi
Copy link
Contributor Author

awesome, cheers, guys!

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.

Tests with Python=3.8 and 3.9 fail
4 participants