Refactor tests

[mayeut]

Similar to #213 but since I started this before it was opened, I finished up what I was doing and this allows to choose a baseline between the 2.

• 1st commit: Use a utils fixture instead of importing utils.
  The utils fixture itself uses the tmp_path fixture in order to be able to work in a specific temporary test directory. All calls to utils.cibuildwheel_run now automatically output wheels in this temporary test directory. A list_wheels() function has been added to utils in order to list the wheels from the correct directory without bothering specifying a path (instead of os.listdir('wheelhouse').

-2nd commit: Run tests with direct pytest invocation
As mentioned in #213, this required to move things around a bit regarding filenames. Still as mentioned in the other PR, one other solution is to add an __init__.py file in each test folder. Yet another solution is to move the cibuildwheel test drivers directly in the test folder.

3rd commit: taken from #213, thanks @Czaki

I kept 2 commits for the utils fixture and allowing pytest direct invocation. I think both should be kept (as in #213), it's much more easier/usual to just be able to run pytest directly.

The first commit revealed an issue in one of the test due to lack of isolation.

[Czaki]

Looks ok, but I'm terrified of next rebase of c++11 on macOS PR #156

[mayeut]

@Czaki,
Let's wait for @joerick and @YannickJadoul input here, they don't necessarily the same view as mine. If you need help rebasing, I'll be ok to do it.

[Czaki]

Maybe add build_dir(self) to _utils class:

def build_dir(self):
    return str(tmp_path)

I do not have any use case for this in this moment, but I strongly expect such interface.

[mayeut]

I was thinking about this but, as you said, there's no use case yet. When it arises, it can be easily added.

[Czaki]

It may be useful during debug. But not in final form of test.

[joerick]

Hi guys! I'm not exactly a pytest pro and I'm not sure how this is different/the same from what we had before. Maybe it's easier to start with - what's the motivation behind these changes?

[Czaki]

@joerick

• using tmp_path fixture:
  no collision with write to same folder. Earlier implementation clean wheelhouse between test groups but person writing test need to do it manually between test in one group. So test_failing_test test in 02_test group has bug. After this test there should not be any wheel, but they are present because of earlier executed test. Using tmp_path take of this problem from programmer head.

• For allow to call pytest instead of bin/run_tests.py:
  Simplified debug. pytest allow to easy select subset of test cases to run. Also use pytest is more natural than searching for scripts launching tests.

Why create custom fixture should write @mayeut.

[YannickJadoul]

Hi there :) Just commented on #213 as well.

Similar to #213 but since I started this before it was opened, I finished up what I was doing and this allows to choose a baseline between the 2.

Thanks! We can take the best parts of the two PRs and make an even better one :)

* 1st commit: Use a `utils` fixture instead of importing `utils`.
  The `utils` fixture itself uses the `tmp_path` fixture in order to be able to work in a specific temporary test directory. All calls to `utils.cibuildwheel_run` now automatically output wheels in this temporary test directory. A `list_wheels()` function has been added to `utils` in order to list the wheels from the correct directory without bothering specifying a path (instead of `os.listdir('wheelhouse')`.

Hmm, I quite like how this hides the dependency on the temporary directory like this! :-) Less typing and less cluttering.
On the other hand, given that utils is mostly used as a module, I'm not entirely a fan of replacing import utils by having a fixture :-/ But that's maybe because I was used to the old code. In the end, it's less error-prone and more consise like this.
Overall, I think I like this part slightly better than @Czaki's, I think, as there's less typing and less chances for mistakes.

-2nd commit: Run tests with direct pytest invocation
As mentioned in #213, this required to move things around a bit regarding filenames. Still as mentioned in the other PR, one other solution is to add an __init__.py file in each test folder. Yet another solution is to move the cibuildwheel test drivers directly in the test folder.

Could you explain which filenames were clashing? Because I liked the fact before that you could copy a template directory before and only needed change the name of the folder. The less test-specific names, the better, no?

3rd commit: taken from #213, thanks @Czaki

I kept 2 commits for the utils fixture and allowing pytest direct invocation. I think both should be kept (as in #213), it's much more easier/usual to just be able to run pytest directly.

The first commit revealed an issue in one of the test due to lack of isolation.

OK, that's pretty cool! If anyone still doubted this was necessary, ... :-)

[Czaki]

@YannickJadoul

Could you explain which filenames were clashing? Because I liked the fact before that you could copy a template directory before and only needed change the name of the folder. The less test-specific names, the better, no?

Clashing names are cibuildwheel_test.py

[YannickJadoul]

@YannickJadoul

Could you explain which filenames were clashing? Because I liked the fact before that you could copy a template directory before and only needed change the name of the folder. The less test-specific names, the better, no?

Clashing names are cibuildwheel_test.py

Ha, and pytest does not distinguish based on the folder? :-( I liked the fact that they all had the same name and that they did not need to be changed every time we make a new test.

[Czaki]

pytest can distinguish base on folder if folder contains __init__.py inside

[YannickJadoul]

Is this env ever used?

[Czaki]

I think that It is needed in test which check that CIBW fail to start if no CI variable is set and CIBW_PLATFORM is not set.

There is no such test now but I think that it should be created.

[YannickJadoul]

It is done automatically by just running cibuildwheel, and test 01_basic also tests it.

[Czaki]

No. Current version of cibuildwheel check on begin if it is running on some of CI or if CIBW_PLATFORM is set. But someone can suggest change which omit this check.

Only possible option for check this in test is to unset some of environment variable. Otherwise after some change it may happen that launching tests will do some changes in host system. Something like:

def test_platoform(utils):
    # check if cibuildwheel fail if runing without CIBW_PATFORM outside CI
    env = os.environ.copy()
    if "CIBW_PLATFORM" in env:
        del env["CIBW_PLATFORM"]
    if "CI" in env:
        del env["CI"]
    if "BITRISE_BUILD_NUMBER" in env:
        del env["BITRISE_BUILD_NUMBER"]
    if "AZURE_HTTP_USER_AGENT" in env:
        del env["AZURE_HTTP_USER_AGENT"]
    with pytest.raises(subprocess.CalledProcessError)
        utils.cibuildwheel_run()

[mayeut]

pytest can distinguish base on folder if folder contains init.py inside

I thought that to from a bug report but unfortunately it's not working...
Need to check why it does not correct the issue. Will do that a bit later.

[mayeut]

This was a bug introduced in a previous PR of mine. The error message a few lines below doesn't get triggered, instead we get a platform is being used before assignment kind of message.

[mayeut]

A test shall probably be added for this.

[Czaki]

here is my suggestion for base

def test_platoform(utils):
    # check if cibuildwheel fail if runing without CIBW_PATFORM outside CI
    env = os.environ.copy()
    if "CIBW_PLATFORM" in env:
        del env["CIBW_PLATFORM"]
    if "CI" in env:
        del env["CI"]
    if "BITRISE_BUILD_NUMBER" in env:
        del env["BITRISE_BUILD_NUMBER"]
    if "AZURE_HTTP_USER_AGENT" in env:
        del env["AZURE_HTTP_USER_AGENT"]
    with pytest.raises(subprocess.CalledProcessError)
        utils.cibuildwheel_run()

checking output can be done in way showed here

[YannickJadoul]

That's not the focus of this PR, though?
Although, yeah, now that a refactoring is being done.

Is this so crucial to test, though?

[mayeut]

Given the various comments about running tests or cibuildwheel itself unadvertently on a dev machine, I'd say it's crucial to test.
My comment "A test shall probably be added for this" is more of a self reminder to create another PR for this. Probably a unit test (which allows for more finer grained things without having to parse output) rather than an integration test.

[YannickJadoul]

Hmmm, myes, OK

[mayeut]

pytest can distinguish base on folder if folder contains __init__.py inside

I thought that to from a bug report but unfortunately it's not working...
Need to check why it does not correct the issue. Will do that a bit later.

Ok so not only the folder has to contain __init__.py, the package name shall be a valid one which was not the case with folders starting with integers: https://stackoverflow.com/questions/17487209/can-a-python-package-name-start-with-a-number

A new test can now be added with a simple copy/paste of an existing one which is a very good thing.

[joerick]

Sorry guys, I'm not convinced of this PR, it's maybe not a clean win quite yet...! Maybe it's more DRY now, but to my eyes, the code looks to be less simple and obvious.

Maybe the following could be improved/resolved? -

• Everything is a python module now. This doesn't feel right to me, and makes the test projects different to the projects that cibuildwheel really builds
• Utils going from an import (familiar to every pythonista) to pytest fixture (mysterious, magical) via the confttest.py file. Not great for discovery, and the utils function that returns a class adds to the confusion (I'm guessing that tmp_path thing is injected by pytest?)
• Utils has state now. Maybe that's a naming thing, but I don't think utils should have state.
• utils.list_wheels() doesn't really describe what's going on. os.listdir('wheelhouse') was obvious.
• Not suuuuper keen on test/test_XX_name/cibuildwheel_test.py conventions... maybe sticking to a postfix _test would be nicer, or tweaking the auto-discovery rules to work with our existing naming system?

Maybe I still don't understand the upside - the material difference I'm seeing so far is built wheels are going to a temp directory which resolves missing cleanup between runs. But to me, this could be resolved with wheelhouse = tempfile.mkdtemp() in each test, and minimal other changes.

I did take a quick look at #213, which might be closer, but many of the above points would apply to to that too.

Happy to hear responses / thoughts on the above!

[mayeut]

@joerick,

Most of the major changes are related to getting pytest direct invocation working.
I can make a PR that just have minimal changes to get each test run in a temporary directory.

As you said yourself

Utils going from an import (familiar to every pythonista) to pytest fixture (mysterious, magical)

For a Python developer, running pytest should be familiar whereas running a mysterious custom script is not to paraphrase what you said.

Let me try to answer each point:

Everything is a python module now. This doesn't feel right to me, and makes the test projects different to the projects that cibuildwheel really builds

If we want to get pytest direct invocation without changing the names of the tests in each directory, then, that's the only way to do it. So either we can change the name of each test (i.e. cibuildwheel_test.py -> cibuildwheel_xx_test.py) or we add an __init__.py. If neither is acceptable, then let's just drop the fact that we can invoke pytest directly to run all tests.

Utils going from an import (familiar to every pythonista) to pytest fixture (mysterious, magical) via the confttest.py file. Not great for discovery, and the utils function that returns a class adds to the confusion (I'm guessing that tmp_path thing is injected by pytest?)

My feeling is that pytest is sufficiently known not to be mysterious or magical, that being said, I'm probably wrong if you're finding this mysterious. Yes, tmp_path is injected by pytest, that's the right way to get a temp directory in pytest.

Utils has state now. Maybe that's a naming thing, but I don't think utils should have state.

Yes, it has a state, if it's just a naming thing, it can be renamed. The fact it has a state allows to simplify each test (but then requires to understand the fixture).

utils.list_wheels() doesn't really describe what's going on. os.listdir('wheelhouse') was obvious.

Again, name could be changed to something more obvious utils.list_built_wheels() or run can return a list of built wheels.

Not suuuuper keen on test/test_XX_name/cibuildwheel_test.py conventions... maybe sticking to a postfix _test would be nicer, or tweaking the auto-discovery rules to work with our existing naming system?

By sticking to a postfix _test I guess you mean test/XX_name_test/cibuildwheel_test.py ? If that's the case it can't be change like that. prefixing XX is the issue here because it's not a valid package name (XX being integers).
As for tweaking the auto-discovery rules, we can tweak pytest to discover about anything (we could have all tests named foo.py for that matter), but we can't make it break existing python rules. We could probably just import all tests modules ourselves with existing tricks in python but IMHO, that would be a very unadvisable thing to do.

[mayeut]

@joerick, I opened 2 other PR #217 / #218 which are much simpler, without all the fuss around pytest direct invocation for all tests. This is what's really needed. pytest direct invocation can be viewed as an enhancement which is not required.

To sum-up pytest direct invocation, which might be done without all the fuss around my moving utils as well (which is not done in #213 as you mentioned but I found test implementation simpler this way), it does require 1 thing, to be able to discover all tests properly.
In order to discover all tests properly, what's required is:

• Use modules from packages (hence adding __init__.py to the test folders)
  or
• Use different filenames in each test folder

For the first, I do understand your point that it makes the test projects different to the projects that cibuildwheel really builds.
For the second, I do understand that it breaks the nice pattern "each test folder has the same structure and a new test can be added with copy/paste, doesn't need to understand why tests broke if you forget to rename the test file."

It seems that pytest direct invocation is not something that can be done without breaking one of those rules so maybe it should not be done at all since it's not required.

[joerick]

Thank you for the detailed explanation @mayeut!

this seems to be the crux, as you say

pytest will find foo/bar/tests/test_foo.py and realize it is NOT part of a package given that there’s no init.py file in the same folder. It will then add root/foo/bar/tests to sys.path in order to import test_foo.py as the module test_foo. The same is done with the conftest.py file by adding root/foo to sys.path to import it as conftest.

https://docs.pytest.org/en/latest/pythonpath.html#standalone-test-modules-conftest-py-files

[joerick]

Thank you @mayeut and @Czaki for the work put in across all these PRs!

[YannickJadoul]

@joerick, I opened 2 other PR #217 / #218 which are much simpler, without all the fuss around pytest direct invocation for all tests. This is what's really needed.

This is great! There's just going on so much in this PR, because @mayeut and @Czaki found a bunch of other things that can be improved (I agree just calling pytest would be nice, but it's indeed not crucial). So just merging in one of the two simpler PRs and fixing the tests would be a great first step? This will make the pytest conversion seem simpler as well, potentially.

[mayeut]

closed in favor of #217