python-cffi / cffi

A Foreign Function Interface package for calling C libraries from Python.
https://cffi.readthedocs.io/en/latest/
Other
133 stars 42 forks source link

rename possibly identical modules to prevent test collisions #135

Closed mattip closed 1 month ago

mattip commented 1 month ago

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

arigo commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Still fails. Maybe differently?

mattip commented 1 month ago

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 commented 1 month ago

Still failing...

mattip commented 1 month ago

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

arigo commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

The latest commit

mattip commented 1 month ago

The latest commit

mattip commented 1 month ago

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.

mattip commented 1 month ago

CI is passing.

arigo commented 1 month ago

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 commented 1 month ago

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

arigo commented 1 month ago

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

mattip commented 1 month ago

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.

arigo commented 1 month ago

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

arigo commented 1 month ago

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