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

rename possibly identical modules to prevent test collisions #135

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Oct 11, 2024

Fixes #134 by renaming the test module, if not already specifically named. I needed two randint invocations to reduce the chances of collision.

@arigo
Copy link
Contributor

arigo commented Oct 11, 2024

I don't know what to do with the failures shown here on macosx. I'm not sure if they are relevant, and if not, I'm not sure how to say "merge anyway please".

@arigo
Copy link
Contributor

arigo commented Oct 11, 2024

Also, not sure why you use randint(32000); I would replace that with randint(1000000000) and still call it twice. It costs nothing and reduces the risks more.

@mattip
Copy link
Contributor Author

mattip commented Oct 11, 2024

OK. I can reproduce the failures locally. That test seems specifically designed to reuse the module, note how the lib2 code is exactly the same as the original one.

@arigo arigo enabled auto-merge (squash) October 11, 2024 08:40
@arigo
Copy link
Contributor

arigo commented Oct 11, 2024

Still fails. Maybe differently?

auto-merge was automatically disabled October 11, 2024 09:04

Head branch was pushed to by a user without write access

@mattip
Copy link
Contributor Author

mattip commented Oct 11, 2024

All of test_verify.py, test_vgen.py, and test_vgen2.py call cffi.verifier.cleanup_tmpdir() at module setup. If another thread/process is running these in parallel, the cleanup may happen when another test is running. But that is a second-order problem, it should only cause slower test runs 🤞

@arigo
Copy link
Contributor

arigo commented Oct 11, 2024

Still failing...

@mattip
Copy link
Contributor Author

mattip commented Oct 11, 2024

CI is passing. I think randomizing module names now makes the test_vgen2.py test irrelevant. Thoughts?

@arigo
Copy link
Contributor

arigo commented Oct 11, 2024

Yes, that's bad. I don't remember any failure caught specifically by test_vgen2, but its point was precisely to reuse the modules.

@arigo
Copy link
Contributor

arigo commented Oct 11, 2024

This dooms the whole approach, I'm afraid. We need either to say pytest-xtest is not supported, or decide test_vgen2 is not very useful and kill it, or maybe find a proper refactoring.

@mattip
Copy link
Contributor Author

mattip commented Oct 11, 2024

Is it important that all the test_vgen tests be rerun without *.c sources, or can I add a specific test for a single case of removing the *.c source and running verify again, and actually check that the source is not regenerated? As far as I can tell, test_vgen2 does not actually check that the source is unneeded, so I am not sure what it is designed to check.

@arigo
Copy link
Contributor

arigo commented Oct 13, 2024

I may originally have decided that I would notice if test_verify2 took as long to execute as test_verify instead of being very fast, just because I was running the tests locally.

If you want to kill the test_v*2.py files and replace them with a single test that checks no C code is regenerated, then I'm fine with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running tests in parallel can cause compilation errors
2 participants