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

Consider nonparametrized tests in reordering #11236

Conversation

sadra-barikbin
Copy link
Contributor

  • To extract fixture dependencies of nonparametrized tests as well, in order to finally consider all fixture dependencies, whether a fixture is parametrized or not, the test is parametrized or not, into reordering.

@sadra-barikbin sadra-barikbin force-pushed the Feature-consider-nonparametrized-tests-in-reordering branch from 62c8ca5 to 01b5db5 Compare August 2, 2023 11:15
@sadra-barikbin
Copy link
Contributor Author

Regarding ubuntu-py38 failure, there were two groups of tests which changed cwd internally but didn't restore it. This would cause test_terminal.py::test_summary_stats to crash, but only if those tests get executed before it, which was not the case up until this PR. Those two groups are

  • The tests that request both pytester and monkeypatch and use monkeypatch.chdir without context, relying on monkeypatch's teardown to restore cwd. This doesn't work because the following sequence of actions take place:
  1. monkeypatch is set up.
  2. pytester (which itself uses monkeypatch) is set up. It saves the original cwd and changes it to a new one dedicated to the test function.
  3. Test function calls chdir of monkeypatch without context. Monkeypatch saves cwd, which is not the original one, before changing it.
  4. Pytester is torn down. It restores the cwd to the original one.
  5. Monkeypatch is torn down. It restores cwd to what it has saved.
    The solution to this is to either use MonkeyPatch.context or call monkeypatch.undo() explicitly at the end of the operations.
  • A couple of tests in _py/test_local.py.

When I fixed these tests, the job passed.

@bluetech
Copy link
Member

Thanks for the PR @sadra-barikbin.

I'm interested to know what is the motivation for this change.

I think (no first hand knowledge, maybe @RonnyPfannschmidt knows) that the restriction of reordering to only parametrized tests is intentional. Consider for example (using tmp_path_factory as just some higher-than-function-scope fixture):

def test_1(tmp_path_factory): pass

def test_2(): pass

def test_3(tmp_path_factory): pass

Without this PR, the ordering is 1, 2, 3. With this PR, the ordering is 1, 3, 2. I think that most users would prefer the existing ordering, and would be confused by the new ordering, which would even put 3 before 2 when it's in an entirely different file. The new ordering goes too far if you catch my meaning.

IMO the guiding principles should be:

  1. The source code order is the "basic" expected ordering...
  2. But, we want to avoid repeated setups/teardowns of higher-than-function-scope fixtures. This mostly only occurs when parametrizing higher-than-function-scope fixtures, and not in non-parametrized functions.

So the way to achieve 2 without hurting 1 too much is to restrict the reordering to parametrized args.

@sadra-barikbin
Copy link
Contributor Author

This PR could be seen as a prelude to the early-teardown feature to finally achieve these:

  1. To reduce the time some resources are allocated, seeing fixtures as resource allocators.
  2. To provide less altered global state to the tests, seeing fixtures as global state changers.

If the above gains happen to be significant, to resolve the issue you mentioned rising from very tmp_path_factory and any fixture like that which itself is a shared resource but its different-per-test effects is used by tests, we could introduce a exclude_from_reordering mark.

@RonnyPfannschmidt
Copy link
Member

the key reason reordering got introduced is to reduce setup/teardown for high scope with parameters for within the scope

from my pov non-parameterized tests ought not to be considered for reordering

@sadra-barikbin
Copy link
Contributor Author

Hi @bluetech @RonnyPfannschmidt , long time no see!🌷 I agree with your arguments. Feel free to close the PR.

@nicoddemus
Copy link
Member

OK thanks @sadra-barikbin for the PR and everyone for the comments. 🙇

Closing this then. 👍

@nicoddemus nicoddemus closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants