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

[tests] Implementation of the new testing plugin (backward-compatible) [part 2] #12093

Open
wants to merge 78 commits into
base: master
Choose a base branch
from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Mar 14, 2024

Follows #12089.

This is the smallest possible set of changes that could be done in order to introduce the new plugin since pytest does not offer the possibilty to enable plugins per conftest file (i.e., I cannot test both the new plugin and keep the old one for the other tests).

As such, I need to merge both plugins (legacy and new implementations) in one plugin. I could like.. ignore the test part of the new plugin but it wouldn't make sense since no one would be able to see the plugin in action otherwise (and just pushing the utility functions does not seem right either).

Once #12089 is merged, this one can follow as it is based on top of the latter. After that, we need to fix each test individually so that they use the new features (for now, the plugin is still backwards compatible, though some parts can be removed when we're done with the cleanup).

cc @jayaddison

@picnixz
Copy link
Member Author

picnixz commented Mar 14, 2024

So, for those interested in reviewing:

There are two major parts in this PR. The first part is the implementation of the plugin itself. Most of the work is done in the internal package where no magic occurs. Markers are processed and arguments are derived according to some strategy that is explained in the comments.

The second part consists in testing the plugin itself. And there's a bit of magic there. As I said before, xdist is capricious. One major drawback is the lack of output capturing. When using pytester, you essentially run an instance of pytest in the process and you can check things directly in the tests you want. Now, imagine you have two tests and you want to be sure that test1 and test2 use different files. The issue is that you cannot test that in test1, nor can you test that in test2 since they need to be executed simultaneously (I want that). So, you would usually print inside the tests, capture the output and compare whatever you want.

The issue is that xdist does not allow you to capture ANYTHING you print using "print". The only way to capture something is to write to the disk (which I don't want to) or use the report sections of pytest + the '-rA' flags. That way, when test1 and test2 are done, the pytest you were running will show the report sections (even if you had used xdist to run test1 and test2 in parallel). And NOW, you can parse that output. This is the job of the "magico" plugin.

Feel free to ask any questions.

@chrisjsewell
Copy link
Member

Heya, as per #12089, could you change your initial comment, to be more specific about what issues this PR proposes to fix, and how it proposes to fix them

@picnixz picnixz marked this pull request as draft March 14, 2024 17:14
@picnixz
Copy link
Member Author

picnixz commented Mar 14, 2024

Sure! it's easier to comment with a new one I think:

So, for now, this PR is meant for preparing the stuff needed for fixing #11285. Currently, this PR only fixes the parts that are "detected" to be bad. More precisely, there are some tests that have side effects because they change their sources (some of them use a shared_result). However it's not good since changing the sources means being independent of every other test (unless you have a parametrized test, in which case, the same change happens for every loop invariant).

There are "3" levels of test isolation. More precisely, the sources directory is constructed as: "TMP_DIR/NAMESPACE/CONFIG/TESTROOT-UNIQUEID" where the NAMESPACE part only depends on the module and the class of the test (i.e., two test functions in the same module are in the same NAMESPACE), "CONFIG" depends on the test configuration (i.e., the parameters passed to pytest.mark.sphinx), "TESTROOT" is the name of the test root being used (just for visual purposes) and "UNIQUEID" is an optional ID to make the directory more unique if needed.

For the level 1 (default), you don't have this "-UNIQUEID" suffix. In addition, the files in "TMP_DIR/NAMESPACE/CONFIG/TESTROOT" will be marked as readonly (but only for this level 1).

Level 2 is only for tests using pytest.mark.parametrize. For those tests, you want to be in the same test directory but you still want to be isolated from the rest. It's as if you have a level 3 isolation where you change the parametrize into a huge for-loop. If you use a level 3, you will have every parametrization in a different folder which makes the test suite very very slow ! it has more or less the same effect using shared_result but where you don't need to care about the shared_result id you choose (which might come bite you after!).

Level 3 ensures that you will never have two tests using the same sources directory. In general, if we cannot decide on the isolation correctly, we use a full isolation.


I'll try to find a way to generate smaller paths on Windows since it doesn't like long path names... Actually, the issue is because of cython which seems to generate a VERY VERY VERY long path so I'll need to investigate.

EDIT: Now, it is still possible to pass an explicit srcdir keyword to the Sphinx marker. If you do that, NAMESPACE and CONFIG will be '-' and 0 respectively and TESTROOT will be changed to that srcdir value, possibly suffixed by some random ID for uniqueness. That way the paths on windows is reduced.

In addition, I reduced the entropy of the random IDs to roughly 32-bits (8 hex chars) since otherwise the paths are too long.

@picnixz
Copy link
Member Author

picnixz commented Mar 14, 2024

Also, the SharedResult class was using a weird construction (like, you use instances for fixtures but use a class variable to hold whatever you want for the whole module). It's fine but it's actually cleaner to use the stash object that pytest suggests instead. Also, having the fixture name clash with the marker argument without being of the same type was confusing for me so I renamed it (shared_result -> module_cache) but kept the old one for backwards compat.

@picnixz
Copy link
Member Author

picnixz commented Mar 14, 2024

Aaaand, finally, I've introduced a better lazy app which does what you think it should do. Like, if you use freshenv=True but use a shared result, it should not be accepted. Same as if you use "force_all" (I explained the rationale at the code level).

sphinx/testing/internal/cache.py Outdated Show resolved Hide resolved
sphinx/testing/internal/markers.py Outdated Show resolved Hide resolved
sphinx/testing/internal/markers.py Outdated Show resolved Hide resolved
sphinx/testing/internal/markers.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@picnixz picnixz marked this pull request as ready for review March 15, 2024 12:46
@picnixz picnixz mentioned this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants