Move script_runner fixture to session scope

[HexDecimal]

Requires its dependencies to also me moved to the session scope, tmp_path was function-scoped and must be replaced with tmp_path_factory.

Needed to work well with custom fixtures, Fixes #89

ScriptRunner attributes marked as Final to document and enforce them as read-only

I've also noticed that rootdir does not seem to be used anywhere, but I didn't do anything with this knowledge. script_cwd and rootdir can probably be removed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.15%. Comparing base (31a7e37) to head (493f406).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   99.54%   98.15%   -1.39%     
==========================================
  Files           1        1              
  Lines         219      217       -2     
==========================================
- Hits          218      213       -5     
- Misses          1        4       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kvas-it]

Hey, @HexDecimal!

Thanks for the fast implementation.

I was thinking that what you did wouldn't work, because when we make script_runner session-scoped, it would be created only once per test session, and then different tests would get the same instance of the fixture, and can't have different launch modes.

It turns out that pytest is smarter than I thought, or perhaps I should say "sneakier". I saw that all the tests pass and my first thought was "this can't be". But they do pass, and all the launch modes are correctly set. So what's going on?

I did some testing, running test_override_launch_mode_with_mark from test_console_scripts.py to understand how it's possible to have different launch modes for different tests if the fixture is session scoped. Sorry for the long log, I hope it's not too boring (otherwise skip to the conclusions at the end).

Preparation

I changed the line before launch_mode_conf to @pytest.fixture(params=[None]) to avoid the test running 4 times. It's not necessary, because the tests override the launch mode anyway and all 4 runs are the same.

I also passed --setup-show to self.pytest.runpytest in RunTest.__call__:

args = ['--setup-show']

Finally, I changed the embedded "test file" to have a session scoped initialization fixture that simulates the heavy setup that we want to only happen once:

import pytest

@pytest.fixture(autouse=True, scope='session')
def setup_fixture(script_runner):
    yield 42

@pytest.mark.script_launch_mode('inprocess')
def test_inprocess(script_runner):
    assert script_runner.launch_mode == 'inprocess'

@pytest.mark.script_launch_mode('subprocess')
def test_subprocess(script_runner):
    assert script_runner.launch_mode == 'subprocess'

@pytest.mark.script_launch_mode('both')
def test_both(script_runner, accumulator=set()):
    assert script_runner.launch_mode not in accumulator
    accumulator.add(script_runner.launch_mode)

Test run 1

I ran pytest -s -k override_launch_mode_with_mark and got this (this is part of the output of the inner pytest run, I skip the irrelevant stuff):

test_override_launch_mode_with_mark.py 
SETUP    S tmp_path_factory
SETUP    S script_cwd (fixtures used: tmp_path_factory)
SETUP    S script_launch_mode['inprocess']
SETUP    S script_runner (fixtures used: script_cwd, script_launch_mode)
SETUP    S setup_fixture (fixtures used: script_runner)
        test_override_launch_mode_with_mark.py::test_inprocess[inprocess] (fixtures used: request,
 script_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
TEARDOWN S setup_fixture
TEARDOWN S script_runner
TEARDOWN S script_launch_mode['inprocess']
SETUP    S script_launch_mode['subprocess']
SETUP    S script_runner (fixtures used: script_cwd, script_launch_mode)
SETUP    S setup_fixture (fixtures used: script_runner)
        test_override_launch_mode_with_mark.py::test_subprocess[subprocess] (fixtures used: reques
t, script_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
TEARDOWN S setup_fixture
TEARDOWN S script_runner
TEARDOWN S script_launch_mode['subprocess']
SETUP    S script_launch_mode['inprocess']
SETUP    S script_runner (fixtures used: script_cwd, script_launch_mode)
SETUP    S setup_fixture (fixtures used: script_runner)
        test_override_launch_mode_with_mark.py::test_both[inprocess] (fixtures used: request, scri
pt_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
TEARDOWN S setup_fixture
TEARDOWN S script_runner
TEARDOWN S script_launch_mode['inprocess']
SETUP    S script_launch_mode['subprocess']
SETUP    S script_runner (fixtures used: script_cwd, script_launch_mode)
SETUP    S setup_fixture (fixtures used: script_runner)
        test_override_launch_mode_with_mark.py::test_both[subprocess] (fixtures used: request, scr
ipt_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
TEARDOWN S setup_fixture
TEARDOWN S script_runner
TEARDOWN S script_launch_mode['subprocess']
TEARDOWN S script_cwd
TEARDOWN S tmp_path_factory

Here we can see that the setup_fixture is created and torn down for each test. Somehow pytest realizes that script_runner needs to be re-created to have the right launch mode, and then setup_fixture is also re-created because it's dependent on script_runner.

Test run 2

Then I removed the marks from the tests so that everything would run with the default launch mode (inprocess):

import pytest

@pytest.fixture(autouse=True, scope='session')
def setup_fixture(script_runner):
    yield 42

def test_inprocess(script_runner):
    assert script_runner.launch_mode == 'inprocess'

def test_subprocess(script_runner):
    assert script_runner.launch_mode == 'inprocess'

def test_both(script_runner, accumulator=set()):
    assert script_runner.launch_mode not in accumulator
    accumulator.add(script_runner.launch_mode)

I ran the same command and got this:

test_override_launch_mode_with_mark.py 
SETUP    S tmp_path_factory
SETUP    S script_cwd (fixtures used: tmp_path_factory)
SETUP    S script_launch_mode['inprocess']
SETUP    S script_runner (fixtures used: script_cwd, script_launch_mode)
SETUP    S setup_fixture (fixtures used: script_runner)
        test_override_launch_mode_with_mark.py::test_inprocess[inprocess] (fixtures used: request,
 script_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
        test_override_launch_mode_with_mark.py::test_subprocess[inprocess] (fixtures used: request
, script_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
        test_override_launch_mode_with_mark.py::test_both[inprocess] (fixtures used: request, scri
pt_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
TEARDOWN S setup_fixture
TEARDOWN S script_runner
TEARDOWN S script_launch_mode['inprocess']
TEARDOWN S script_cwd
TEARDOWN S tmp_path_factory

This is expected behavior with scope="session". The runner is created only once and setup_fixture is also created once.

Test run 3

I tried one more variation, where I only had one test with a mark:

import pytest

@pytest.fixture(autouse=True, scope='session')
def setup_fixture(script_runner):
    yield 42

def test_inprocess(script_runner):
    assert script_runner.launch_mode == 'inprocess'

def test_subprocess(script_runner):
    assert script_runner.launch_mode == 'inprocess'

@pytest.mark.script_launch_mode('both')
def test_both(script_runner, accumulator=set()):
    assert script_runner.launch_mode not in accumulator
    accumulator.add(script_runner.launch_mode)

The output was like this:

test_override_launch_mode_with_mark.py 
SETUP    S tmp_path_factory
SETUP    S script_cwd (fixtures used: tmp_path_factory)
SETUP    S script_launch_mode['inprocess']
SETUP    S script_runner (fixtures used: script_cwd, script_launch_mode)
SETUP    S setup_fixture (fixtures used: script_runner)
        test_override_launch_mode_with_mark.py::test_inprocess[inprocess] (fixtures used: request,
 script_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
        test_override_launch_mode_with_mark.py::test_subprocess[inprocess] (fixtures used: request
, script_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
        test_override_launch_mode_with_mark.py::test_both[inprocess] (fixtures used: request, scri
pt_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
TEARDOWN S setup_fixture
TEARDOWN S script_runner
TEARDOWN S script_launch_mode['inprocess']
SETUP    S script_launch_mode['subprocess']
SETUP    S script_runner (fixtures used: script_cwd, script_launch_mode)
SETUP    S setup_fixture (fixtures used: script_runner)
        test_override_launch_mode_with_mark.py::test_both[subprocess] (fixtures used: request, scr
ipt_cwd, script_launch_mode, script_runner, setup_fixture, tmp_path_factory).
TEARDOWN S setup_fixture
TEARDOWN S script_runner
TEARDOWN S script_launch_mode['subprocess']
TEARDOWN S script_cwd
TEARDOWN S tmp_path_factory

This is smart, the runner and our heavy session-scoped fixture are created once for each launch mode, and then the tests are grouped by launch mode. Perhaps they are not really grouped though, pytest just notices when it doesn't need to re-create the fixtures because the one from the previous test will do.

Conclusions

1. Pytest automatically re-creates session-scoped fixtures if their dependency needs to be re-created. Changing script_runner to session scope is safe and does not break launch mode setting. Also it behaves like a session-scoped fixture if the launch mode is not set.
2. Unfortunately this means that if we have a heavy session-scoped fixture depending on script_runner, it will be re-created for each launch mode used in the tests. We probably should document this to not surprise users.
3. Sadly, pytest is not always smart enough to group the tests by launch mode and then instantiate script_runner and its dependencies only once per launch mode. Sometimes it re-instantiates them unnecessarily.
4. Maybe there's a way to change the launch mode dynamically without re-creating the script_runner.
   a. We could add an argument to script_runner.run to override launch modes -- then it could be used instead of the marks. This would be new API, not breaking anything, but kind of confusing.
   b. We could try to figure out a way to keep script_launch_mode fixture function-scoped, remove the explicit dependency from script_runner to it, so that script_runner remains session-scoped, and use some black magic to get the current launch mode inside script_runner.run(). I'm not sure how exactly to do this, but maybe there's a way.

Overall, I think that this PR should not break anything for anyone or make anything worse than before. Now it's possible to use script_runner in session-scoped fixtures, but it can't be mixed with launch mode marks, otherwise session-scoped dependents of script_runner will be re-created for each launch mode (potentially multiple times).

Would be good to document how this works. Also, if you have ideas for point 4 above, maybe we can improve this further.

Cheers,
Vasily

[HexDecimal]

It seems the request fixture might have its own complex rules. After I replaced tmp_path then everything else kind of fell into place.

It was enlightening to have a clear confirmation on how this works. You've already put more work into this PR than I did.

Unfortunately this means that if we have a heavy session-scoped fixture depending on script_runner, it will be re-created for each launch mode used in the tests. We probably should document this to not surprise users.

This seems like the desired behavior even when the fixture is a heavy one.

Sadly, pytest is not always smart enough to group the tests by launch mode and then instantiate script_runner and its dependencies only once per launch mode. Sometimes it re-instantiates them unnecessarily.

That's unfortunate. Though it seems to be an issue with pytest itself and I'd rather report the behavior to pytest than try to make a local workaround. Maybe there's something simple that's being missed.

[kvas-it]

Unfortunately this means that if we have a heavy session-scoped fixture depending on script_runner, it will be re-created for each launch mode used in the tests. We probably should document this to not surprise users.

This seems like the desired behavior even when the fixture is a heavy one.

Yeah, you're right, it's logical to run the session-scoped fixture with the right launch mode.

Sadly, pytest is not always smart enough to group the tests by launch mode and then instantiate script_runner and its dependencies only once per launch mode. Sometimes it re-instantiates them unnecessarily.

That's unfortunate. Though it seems to be an issue with pytest itself and I'd rather report the behavior to pytest than try to make a local workaround. Maybe there's something simple that's being missed.

My impression is that pytest doesn't reorder the tests to minimise fixture creation. It would be fairly hard to do it well, given that there could be multiple session-but-not-session-scoped fixtures with different rules and unknown execution times. Also I imagine that session-scoped fixtures that need to be created more than once are themselves a bit of a corner case, so pytest tries to make things work correctly but asking it to also be max efficient might be a bit too much.

Perhaps we could report and discuss with pytest developers, but for this PR I would add a bit of explanation to the README and then merge it -- it should solve #89 and perhaps help some other people too.

[thekswenson]

Thank you folks!

[HexDecimal]

metafunc.parametrize was missing a scope argument:

scope (Optional[_ScopeName]) – If specified it denotes the scope of the parameters. The scope is used for grouping tests by parameter instances. It will also override any fixture-function defined scope, allowing to set a dynamic scope using test context or configuration.

I suspect that adding this will give pytest enough context to fix the grouping issues.

@kvas-it can you verify that my last commit works correctly?

[kvas-it]

metafunc.parametrize was missing a scope argument:

scope (Optional[_ScopeName]) – If specified it denotes the scope of the parameters. The scope is used for grouping tests by parameter instances. It will also override any fixture-function defined scope, allowing to set a dynamic scope using test context or configuration.

I suspect that adding this will give pytest enough context to fix the grouping issues.

Unfortunately there's no change. Pytest doesn't seem to reorder the tests to optimise the use of our fixture.

However, I learned today that we can do it ourselves -- there's pytest_collection_modifyitems hook that we can implement to group the tests by launch mode.

Reordering the tests seems quite invasive for a plugin like ours, but we could also make it optional (controlled by a command line option) and by default only notice that fixtures are re-created, report any session-scoped fixtures dependent on the runner and how many times they were created and what would be the optimal number of times (maybe print a warning, pytest has mechanisms for that).

All of this is great, but also with regard to this PR, my preference is probably to just document the current behavior and merge it. We could recommend something like pytest-order as a workaround for now. Then maybe create the ticket about re-ordering feature and see if we get to implementing it.

What do you think?