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

Closed
wants to merge 14 commits into 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.

@mattip
Copy link
Contributor Author

mattip commented Oct 13, 2024

The latest commit

  • removes the test_v*2.py files and adds a test_no_regen that checks the C file is not regenerated
  • moves the global verifier._TMPDIR and verifier._FORCE_GENERIC_ENGINE into a threading.local() to make the global state more thread-safe
  • refactors compilation to lessen the likelyhood of creating a filename that is longer than MAXPATH on windows.

@mattip
Copy link
Contributor Author

mattip commented Oct 13, 2024

The latest commit

  • uses filelock to make test_parse_c_type build lib only once
  • restores verifier.sourcefilename after shortening it for compilation
  • documents the need for filelock
  • runs tests with pytest-xdist -n 4 and add back windows testing

@mattip
Copy link
Contributor Author

mattip commented Oct 13, 2024

Adding back windows testing almost worked, I had to skip embedded tests. I will open an issue so if someone is so inclined they can fix it. The tests are failing on the PyPy windows buildbot as well.

@@ -54,8 +55,7 @@ Download and Installation:
``git clone https://github.com/python-cffi/cffi``

* running the tests: ``pytest c/ testing/`` (if you didn't
install cffi yet, you need first ``python setup_base.py build_ext -f
-i``)
install cffi yet, you need first ``python -m pip install -e .``)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These days pip install is to be preferred

testing/cffi0/test_verify.py Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented Oct 13, 2024

CI is passing.

@arigo
Copy link
Contributor

arigo commented Oct 17, 2024

I can approve this without looking in details as it mostly touches tests. Can you confirm the underlying reason, though: why do we want to run pytest-xdist? The negative points are that it creates issues and adds extra installation dependencies; what is the positive point?

@mattip
Copy link
Contributor Author

mattip commented Oct 17, 2024

On windows, tests now run in CI in ~11 minutes, where before they were 45 minutes.

@arigo
Copy link
Contributor

arigo commented Oct 18, 2024

It would be very nice, if cffi was actively being developed. Bu as it is not, my point stands..?

@mattip
Copy link
Contributor Author

mattip commented Oct 18, 2024

these tests are run by pypy (in its extra tests) for every nightly buildbot run. The windows runs are very slow. So these changes are more for the PyPy test than for cffi itself, trying to minimize the diff between the two copies. But I see your point and will close this.

@mattip mattip closed this Oct 18, 2024
@arigo
Copy link
Contributor

arigo commented Oct 18, 2024

Oh, if it makes the nightly test runs of pypy more than 30 minutes faster, that's an interesting outcome.

@arigo
Copy link
Contributor

arigo commented Oct 18, 2024

Should I review this? Or wait for #137 (which apparently needs some fixing)?

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