Skip to content

Use pytest-randomly for reproducible random test parameters#5868

Merged
CirqBot merged 16 commits intoquantumlib:masterfrom
pavoljuhas:use-pytest-randomly
Sep 20, 2022
Merged

Use pytest-randomly for reproducible random test parameters#5868
CirqBot merged 16 commits intoquantumlib:masterfrom
pavoljuhas:use-pytest-randomly

Conversation

@pavoljuhas
Copy link
Copy Markdown
Collaborator

@pavoljuhas pavoljuhas commented Sep 8, 2022

  • Require pytest-randomly for the testing to ensure RNG-generated test
    parameters are consistent across parallel test jobs.
  • Remove check/pytest option --actually-quiet which disables pytest-randomly
    hook for seeding parallel test jobs.
  • Remove CIRQ_TESTING_RANDOM_SEED as it is not needed anymore.
  • Remove RNG seeding in the tests where it seems redundant.
  • Clean up one instance of unnecessary test parametrization.

This upholds #4787 and replaces #4788.
Reverts #1825 and obsoletes #1826.

These instances of seeding do not appear to affect test results.
No need to parametrize for a single set of values,
let us set them in the test body instead.
Replaced by pytest-randomly.  The seed from pytest-randomly appears to
be in effect at the test discovery stage and makes RNG-generated test
parameters reproducible.
@pavoljuhas pavoljuhas requested review from a team, cduck and vtomole as code owners September 8, 2022 23:55
@pavoljuhas pavoljuhas requested a review from viathor September 8, 2022 23:55
@CirqBot CirqBot added the size: S 10< lines changed <50 label Sep 8, 2022
Avoid using `pytest --quiet` which disables pytest-randomly hook that
configures consistent seeding for parallel test jobs.

Reverts quantumlib#1825.
@pavoljuhas
Copy link
Copy Markdown
Collaborator Author

@vtomole - can you PTAL at this PR? Let me know if there are any questions. Thanks!

Copy link
Copy Markdown
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

Thanks for the ping. Forgot to submit review: LGTM.

@vtomole
Copy link
Copy Markdown
Collaborator

vtomole commented Sep 20, 2022

What happened to #4788 (comment)?

@vtomole
Copy link
Copy Markdown
Collaborator

vtomole commented Sep 20, 2022

#5880

@pavoljuhas
Copy link
Copy Markdown
Collaborator Author

What happened to #4788 (comment)?

I believe I was checking the tests with check/pytest script which was executing pytest --quiet ....
pytest-randomly needs to do seeding before the test discovery so that the seed is active at import time for the RNG-dependent test parameters. This happens in the pytest_report_header hook, which is however disabled by the --quiet option.

In short, pytest --quiet breaks seeding of test parameters by pytest-randomly and with that also parallel testing (test jobs get different tests). Things are fine without the -q, --quiet option, so this PR removes it in 13890c6.

@pavoljuhas pavoljuhas added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 20, 2022
@CirqBot
Copy link
Copy Markdown
Contributor

CirqBot commented Sep 20, 2022

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 20, 2022
@pavoljuhas pavoljuhas added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 20, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Sep 20, 2022
@CirqBot CirqBot merged commit 8ff2ce0 into quantumlib:master Sep 20, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Sep 20, 2022
@pavoljuhas pavoljuhas deleted the use-pytest-randomly branch September 20, 2022 21:28
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…ib#5868)

- Require pytest-randomly for the testing to ensure RNG-generated test
  parameters are consistent across parallel test jobs.
- Remove check/pytest option --actually-quiet which disables pytest-randomly
  hook for seeding parallel test jobs.
- Remove `CIRQ_TESTING_RANDOM_SEED` as it is not needed anymore.
- Remove RNG seeding in the tests where it seems redundant.
- Clean up one instance of unnecessary test parametrization.

This upholds quantumlib#4787 and replaces quantumlib#4788.
Reverts quantumlib#1825 and obsoletes quantumlib#1826.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
…ib#5868)

- Require pytest-randomly for the testing to ensure RNG-generated test
  parameters are consistent across parallel test jobs.
- Remove check/pytest option --actually-quiet which disables pytest-randomly
  hook for seeding parallel test jobs.
- Remove `CIRQ_TESTING_RANDOM_SEED` as it is not needed anymore.
- Remove RNG seeding in the tests where it seems redundant.
- Clean up one instance of unnecessary test parametrization.

This upholds quantumlib#4787 and replaces quantumlib#4788.
Reverts quantumlib#1825 and obsoletes quantumlib#1826.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: S 10< lines changed <50

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants