Skip to content

Remove non deterministic test param#1138

Merged
TheRealFalcon merged 4 commits into
canonical:mainfrom
holmanb:holmanb/remove-non-deterministic-test-param
Dec 7, 2021
Merged

Remove non deterministic test param#1138
TheRealFalcon merged 4 commits into
canonical:mainfrom
holmanb:holmanb/remove-non-deterministic-test-param

Conversation

@holmanb
Copy link
Copy Markdown
Member

@holmanb holmanb commented Dec 7, 2021

Remove non-deterministic test parameter
    
Non-deterministic test parameters have multiple downsides:
 - potentially non-deterministic tests results
 - breaks parallel test execution via xdist.

Additional Context

There are other things in our unittests that cause intermittent failures during parallel test execution, but this one will cause failures every time. The random input here is frankly unnecessary. With this change I can (usually) run pytest tests/unittests -n2 locally in ~25 seconds, rather than the normal >40s to complete.

This breaks parallel test execution via xdist.
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this specific instance I agree that the randomness doesn't really add much, but in general I think "any value of this type" should be considered valid input for a test. The failure is because parametrized test names are auto-generated based on the parameters.

This can also be fixed with pytest.param(CiTestCase.random_string(), None, id="random_string"),. In this case I'm fine with either one, but in general, I think I'd prefer we keep random parameters and assign ids to parameters.

With this change I can (usually) run pytest tests/unittests -n2 locally in ~25 seconds, rather than the normal >40s to complete.

Lucky...mine are double that 😄

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know which you prefer for this test.

@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Dec 7, 2021

In this specific instance I agree that the randomness doesn't really add much, but in general I think "any value of this type" should be considered valid input for a test. The failure is because parametrized test names are auto-generated based on the parameters.

This can also be fixed with pytest.param(CiTestCase.random_string(), None, id="random_string"),. In this case I'm fine with either one, but in general, I think I'd prefer we keep random parameters and assign ids to parameters.

Good info, thanks! I'll update the PR to set the parameterized test name.

@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Dec 7, 2021

@TheRealFalcon looks like param doesn't exist on some of the versions of pytest that we use. I don't see reference to a change in the api in the docs, however. Unless there are any objections to this iteration I'm fine with this approach.

@TheRealFalcon TheRealFalcon merged commit b21afb0 into canonical:main Dec 7, 2021
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.

2 participants