Custom test checks alternate #137
Conversation
on how they're run, and combine environment.json and check.py
|
Cool! Using pytest is a great improvement indeed! I'm still not 100% convinced of getting rid of I'll have a fuller look once I'm behind my laptop screen, but do close #136 if you are happy with this PR :-) |
test/01_basic/cibuildwheel_test.py
Outdated
|
|
||
| # check that every wheel is produced | ||
| if utils.platform == 'linux': | ||
| assert glob('wheelhouse/*-cp27-cp27m-manylinux1_x86_64.whl') |
There was a problem hiding this comment.
What about adding all filenames to utils (possibly in a function that takes project and version parameters, and then generates the filename), and then checking that the (filtered, if not all wheels are being built) set of expected filenames matches the set of filenames found by a single glob('wheelhouse/*.whl')? (I think pytest will give you a diff between the two sets, if that assert fails.)
There was a problem hiding this comment.
What about adding all filenames to utils (possibly in a function that takes project and version parameters, and then generates the filename),
ooh.... i'm maybe... 50/50 on this. I like the explicitness of how it's currently written, it's very clear what's being looked for and the logic is fully within the test. But a set comparison might be neater and we could run it for every test, not just these two. I might try an implementation and see how it feels
There was a problem hiding this comment.
Fair enough; just an idea. Indeed, if this works for the basic test and the tests on skipping, that should be enough, and the other tests can focus on something else. I was also just thinking of getting rid of the --print-build-identifiers dependency in the test, since there could be errors in there as well?
test/01_basic/cibuildwheel_test.py
Outdated
| import subprocess, sys, os | ||
| from glob import glob | ||
| project_dir = os.path.dirname(__file__) | ||
| sys.path.append(os.path.join(os.path.dirname(project_dir), 'shared')) |
There was a problem hiding this comment.
Could we set this in the test driver, since it's (by definition) shared by all tests anyway? That's one less line to copy when adding a new test/one less line to change in every test file if there's a bug.
Adding (or appending to) the PYTHONPATH environment variable could work, here?
There was a problem hiding this comment.
Hm, yes I agree. PYTHONPATH is a good idea! I'll look at changing that.
|
thanks for the review, all good points.
On the test framework enforcing a methodology - I think it's a good point, but I'm not sure it's worth the complexity of splitting the test setup and check across two different files - much nicer to have cause and effect right there in one file IMO. But I agree it's a taste thing - I also wasn't keen on the 'environment as a JSON file' thing - never felt all that pleasant to edit JSON files 🤷♂️ |
test/01_basic/cibuildwheel_test.py
Outdated
| project_dir = os.path.dirname(__file__) | ||
|
|
||
| # build the wheels | ||
| subprocess.check_call([sys.executable, '-m', 'cibuildwheel', project_dir]) |
There was a problem hiding this comment.
Minor idea: what about moving this into utils as well? utils.call_cibuildwheel() or so? This gets rid of import subprocess, sys, os as well, and then the test has even more focus on the things we're checking.
There was a problem hiding this comment.
(OK, I now see you'd need an env parameter, or add_env, with a dictionary of things to update)
There was a problem hiding this comment.
yes! I was just now working on something very similar....
There was a problem hiding this comment.
Yesss, great :-)
For me this takes away the burden that cibuildwheel isn't called from run_test.py, if it's just a single line to write anywa.
|
Great! I've closed #136, because I think this updated version clearly has advantages, so let's continue on this :-) |
Also, wrap the cibuildwheel call to make building env easier
|
This might now be ready, if it looks good to you @YannickJadoul ? |
Building on #136 - main differences are