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

Don't reseed before test calls #19

Closed
wants to merge 1 commit into from

Conversation

rouge8
Copy link

@rouge8 rouge8 commented Sep 9, 2016

Thanks for this plugin, it's definitely made our tests better. However, I think we've run into a bug when using fixtures with random data.

This patch changes the reseeding behavior to only reseed at test setup/teardown, but not again when running the test. This means that the state will no longer be reset between a fixture and the test using the fixture, which enables tests like this to work as expected:

import random
import pytest

@pytest.fixture
def random_number():
    return random.random()

def test_something(random_number):
    new_number = random.random()
    assert new_number != random_number

This example is simple, but becomes important when fixtures use Factory-Boy to generate more complex random data.

@adamchainz adamchainz self-assigned this Sep 10, 2016
@adamchainz
Copy link
Member

Why do you want this with factory boy? So that your fixtures can generate data with a factory and then the test method can use the factory again and get (probably) different data?

Re-setting at the start of the test is important with test ordering since otherwise running one or two tests will give different results due to the random state not being reset. I don't think we can blanket remove it. If you really think it's a good idea though you could add a flag.

@rouge8
Copy link
Author

rouge8 commented Sep 10, 2016

Why do you want this with factory boy? So that your fixtures can generate data with a factory and then the test method can use the factory again and get (probably) different data?

Exactly that.

Re-setting at the start of the test is important with test ordering since otherwise running one or two tests will give different results due to the random state not being reset

It will still be reset for each test though right? My understanding is that pytest_runtest_setup is called for each test.

I don't think we can blanket remove it. If you really think it's a good idea though you could add a flag.

A coworker actually wrote the flag version first. I think the no-flag version accomplishes everything, but if you disagree we can turn that into a PR too. :)

@adamchainz
Copy link
Member

I dug around on the poorly documented pytest_runtest_setup and found that it's called before any fixtures are set up, and then pytest_runtest_call is run before the actual test method is invoked. See the test I just added in #21 - removing the pytest_runtest_call hook causes it to fail, which shows that the tests wouldn't be isolated properly.

Perhaps an alternative solution would be to reset the random seed at the start of the test to a different value than the one used in fixtures - or even just advance the random generator a bit by calling e.g. random.random() N times. As long as it's deterministic it should be good.

This patch changes the reseeding behavior to only reseed at test
setup/teardown, but not again when running the test. This means that the
state will no longer be reset between a fixture and the test using the
fixture, which enables tests like this to work as expected:

```python
import random
import pytest

@pytest.fixture
def random_number():
    return random.random()

def test_something(random_number):
    new_number = random.random()
    assert new_number != random_number
```

This example is simple, but becomes important when fixtures use
Factory-Boy to generate more complex random data.
@rouge8
Copy link
Author

rouge8 commented Sep 11, 2016

See the test I just added in #21 - removing the pytest_runtest_call hook causes it to fail, which shows that the tests wouldn't be isolated properly

Ah yeah, I hadn't considered how non-function scoped fixtures would affect this.

I think I like the idea of resetting the seed in pytest_runtest_call to a different value than pytest_runtest_setup. It would have to have some relationship to --randomly-seed I guess.

Thanks for digging into this more...

@adamchainz
Copy link
Member

No worries. Closing this PR in favour of a different solution in #22 .

@adamchainz adamchainz closed this Sep 11, 2016
@adamchainz
Copy link
Member

@rouge8 can you check #23 and confirm it would help with your fixtures? 😄

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

2 participants