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

Mark and skip particularly slow unit tests #1001

Merged
merged 9 commits into from
Sep 18, 2019
Merged

Conversation

braised-babbage
Copy link
Contributor

@braised-babbage braised-babbage commented Sep 18, 2019

Description

A few tests dominate the runtime of the test suite, and this makes me less likely to run tests. On my system,

== slowest 10 test durations ==
45.54s call     pyquil/tests/test_quantum_computer.py::test_run_symmetrized_readout_error
40.33s call     pyquil/tests/test_reference_density_simulator.py::test_for_negative_probabilities
15.67s call     pyquil/tests/test_operator_estimation.py::test_measure_observables_many_progs
6.24s call     pyquil/tests/test_quantum_computer.py::test_run_pyqvm_noisy
4.40s call     pyquil/tests/test_operator_estimation.py::test_measure_observables_symmetrize_calibrate
4.34s call     pyquil/tests/test_operator_estimation.py::test_2q_unitary_channel_fidelity_readout_error
2.97s call     pyquil/tests/test_operator_estimation.py::test_measure_observables
2.83s call     pyquil/tests/test_operator_estimation.py::test_measure_observables_symmetrize
2.33s call     pyquil/tests/test_operator_estimation.py::test_measure_observables_result_zero_symmetrization_calibration
2.26s call     pyquil/tests/test_operator_estimation.py::test_sic_process_tomo
== 596 passed, 6 skipped, 1 xfailed, 36 warnings in 160.55s (0:02:40) ==

This PR adds a --include-slow-tests flag, as well as a marker pytest.marks.slow for indicating that certain tests are slow. I've flagged the top 3 in the above list. If we decide to move forward with this idea, probably we need to configure gitlab to add the --include-slow-tests (since these things should be tested for a PR, for example).

Checklist

  • The above description motivates these changes.
  • There is a unit test that covers these changes.
  • All new and existing tests pass locally and on Semaphore.
  • Parameters have type hints with PEP 484 syntax.
  • Functions and classes have useful sphinx-style docstrings.
  • (New Feature) The docs have been updated accordingly.
  • (Bugfix) The associated issue is referenced above using
    auto-close keywords.
  • The changelog is updated,
    including author and PR number (@username, gh-xxx).

@notmgsk
Copy link
Contributor

notmgsk commented Sep 18, 2019

hm

@karalekas
Copy link
Contributor

wow I drop it from 40 to 3 and it's still too slow for you? :P

Copy link
Contributor

@karalekas karalekas left a comment

Choose a reason for hiding this comment

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

I'd be fine taking this route in the near-term to lower the barrier of entry for developers. You should update the gitlab YAML to run the slow tests, and add some information to the contribution guide about this option and which tests specifically are marked as slow.

pyquil/tests/conftest.py Show resolved Hide resolved
@karalekas
Copy link
Contributor

Also need to update changelog ofc

@braised-babbage
Copy link
Contributor Author

Alternatively we can leave this to users. I was unaware of pytest -k, which allows for an easy command-line deselection of tests, e.g. pytest -k 'not symmetrized_readout and not negative_probabilities'

Copy link
Contributor

@karalekas karalekas left a comment

Choose a reason for hiding this comment

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

if we do this, let's just use the canonical --runslow

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@karalekas karalekas left a comment

Choose a reason for hiding this comment

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

tiny tiny stuff -- also can you move your changelog entry to the bottom of the "improvements and changes" list? each section is pretty much chronological by merge date. otherwise LGTM!

CHANGELOG.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Erik Davis and others added 2 commits September 18, 2019 14:29
Co-Authored-By: Peter Karalekas <peter@rigetti.com>
@braised-babbage
Copy link
Contributor Author

tiny tiny stuff -- also can you move your changelog entry to the bottom of the "improvements and changes" list? each section is pretty much chronological by merge date. otherwise LGTM!

Good catches!

Copy link
Contributor

@karalekas karalekas left a comment

Choose a reason for hiding this comment

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

🎉

@karalekas karalekas changed the title Feature/skip slow tests Mark and skip particularly slow unit tests Sep 18, 2019
@karalekas karalekas added this to the v2.12 milestone Sep 18, 2019
@karalekas karalekas added the devops 🚀 An issue related to CI/CD. label Sep 18, 2019
@karalekas karalekas merged commit 6e8936a into master Sep 18, 2019
@karalekas karalekas deleted the feature/skip-slow-tests branch September 18, 2019 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops 🚀 An issue related to CI/CD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants