Run each test using a temporary directory (using tmp_path)

[mayeut]

Simpler way of getting each test to run in a unique temporary directory than #213 and #214 which also attempts to make all tests being run with a single pytest invocation.

[mayeut]

Similar to 370225e commit by @Czaki

[joerick]

Let's go with this for now, being the simplest version, and then consider some improvements to the test runner as @Czaki has started in #213 - not stopping on the first failure, and perhaps hiding command output when test is successful.

[Czaki]

In my PR #213 i use test/conftest.py to update sys.path This change allow to launch single test with pytest, but not change the workflow. Is this move of place of update python path is ok for you @joerick ?

import os
import sys 

sys.path.append(os.path.dirname(__file__))

And why keep order of tests is important? Remove numbering will simplify everything.

[joerick]

@Czaki interesting... Does that still require changing the name of each test file to be unique?

[Czaki]

@joerick No. Because the directory names are proper python package names (no numbers on begin), then they are disjoined by directory name.

I refactor #213 to show this (there is need of change ssl test name because of collision with python module ssl)

There could be used also other conventions of name test group, like prefix test_, or prefix test_{number}_ to get proper sorting.

[mayeut]

@Czaki interesting... Does that still require changing the name of each test file to be unique?

@joerick No. Because the directory names are proper python package names (no numbers on begin), then they are disjoined by directory name.

Right, but now it needs an __init__.py in each test folder. As mentioned in #214, it's either one or the other to get pytest being able to call all tests at once.

[YannickJadoul]

import os
import sys 

sys.path.append(os.path.dirname(__file__))

I do slightly object against having to do this in every test. That's just boilerplate code that's unrelated to the test, and repeating it each time distracts.

(I do know this PR doesn't add that.)

Let's go with this for now, being the simplest version

Why do you consider this to be simpler than #217? I somehow had the opposite feeling. It's just a feeling, but if we need to do tmp_path and str(tmp_path) for every test, why not make it part of cibuildwheel_run? Then there's no chance to forget it.
And to some degree, it even makes sense that cibuildwheel_run would return a list of wheels (or well, wheel names), no?

[YannickJadoul]

I do slightly object against having to do this in every test. That's just boilerplate code that's unrelated to the test, and repeating it each time distracts.

Sorry, my bad. I'm only now looking at the updates on #213, @Czaki. Nicely done! :-)

[mayeut]

Why do you consider this to be simpler than #217? I somehow had the opposite feeling. It's just a feeling, but if we need to do tmp_path and str(tmp_path) for every test, why not make it part of cibuildwheel_run? Then there's no chance to forget it.
And to some degree, it even makes sense that cibuildwheel_run would return a list of wheels (or well, wheel names), no?

That's my feeling too. I think the only reason I came up with the tmp_path PR was because of the only comment I thought I'd get on the other one (and @YannickJadoul was spot on with his comment)

[Czaki]

@YannickJadoul @mayeut I think that hiding path to output dir is wrong idea. At least for debug purpose.

[mayeut]

@YannickJadoul @mayeut I think that hiding path to output dir is wrong idea. At least for debug purpose.

@Czaki,
I don't see exactly what wouldn't be achievable when debugging. Can you give an example for me to better understand please ?

[Czaki]

When new release of wheel will be released then access to wheel may be useful to understand why wrong tag is created.

Hiding information from developer is bad for me.

[mayeut]

When new release of wheel will be released then access to wheel may be useful to understand why wrong tag is created.
Hiding information from developer is bad for me.

I still don't see how it's hidden and the developer can't access it. It will only require stepping into the run command or moving a breakpoint.

Regarding my comment about:

@Czaki interesting... Does that still require changing the name of each test file to be unique?

@joerick No. Because the directory names are proper python package names (no numbers on begin), then they are disjoined by directory name.

Right, but now it needs an __init__.py in each test folder. As mentioned in #214, it's either one or the other to get pytest being able to call all tests at once.

I missed the bit about adding contest.py allowing to run a single test with pytest without bothering to set PYTHONPATH. This is indeed a good addition to make (simpler in an IDE).

[Czaki]

So main difference between these PR and #213 is rename of folders to allow run whole dataset with pytest?

[mayeut]

yes

[YannickJadoul]

@YannickJadoul @mayeut I think that hiding path to output dir is wrong idea. At least for debug purpose.

@Czaki,
I don't see exactly what wouldn't be achievable when debugging. Can you give an example for me to better understand please ?

Yeah, if we don't print the value of the tmp_dir fixture, that's just as hidden, no?
Moreover, if wheels get written there, the shell commands that are executed by cibuildwheel still get printed. And maybe we could add a line in utils.run_cibuildwheel printing the full command being run? Then it's rather obvious what temp folder is used.

The main thing I'm aiming for is to have a single test be as simple as it can be. Trying to put overhead in a single place, utils.py or conftest.py. Because any repeated code will only lead to extra work at best or inconsistencies at worst. (That's why I didn't want sys.path.append in each test, and why I feel I like #217 a bit better.)

If we commit to going fully with pytest, then of course the whole tmp_dir fixture inside of a utils fixture or so becomes quite interesting again, I'd say. But if we keep pytest things to the minimum, I really like the manual temporary dir in utils, used by default unless manually overwritten by the user of the test for a specific reason relevant to that test.

[YannickJadoul]

So main difference between these PR and #213 is rename of folders to allow run whole dataset with pytest?

Yes, but @joerick is not convinced of the full switch to pytest, so it's better to split up the PR in the two main parts: first, a fix for the temporary directories that you spotted; and then afterwards we can discuss the pytest. But it's clearer for @joerick to judge if we keep these two independent things separated :-)

[joerick]

From my perspective, it's pretty marginal differences - either #217 or #218 are both good :) It seems that @mayeut and @YannickJadoul are preferring #217, so shall we go with that? Feel free to merge that if you like @mayeut.

[mayeut]

Per @joerick comment, closing in favor of #217