Skip slow test configurations for slow tests #463
Conversation
|
To be sure, these tests can still be run if we provide a flag like There's a parallel effort happening in Cirq to solve this problem where we can mark tests as "slow" and they'll be run only as part of daily / weekly CI instead of running them on every PR. |
|
This PR doesn't respond to flags; it just skips one of the configurations. You can run it by commenting out the skip which honestly is as good as a command line flag because I would bet most of the slow tests in projects with an |
The way it's handled in Cirq is that the slow tests get executed once as part of the daily CI while the rest of the tests get executed as part of every PR. I think this model is more robust and should be adopted. The reason slow tests exist is because we have found implementation bugs in the past which were not caught by small test cases, and having the daily CI running these slow tests will help us catch such bugs in case we change the implementation. The "run it by commenting out the skip" is not nearly as robust. Also, the infrastructure to add the daily CI and mark tests as slow is not terribly complicated and already exists in Cirq now; thanks to @pavoljuhas; so we can leverage the knowledge we've gained by implementing it in Cirq to simply adopt the same infrastructure here as well. |
|
Ok, let's merge this until the nightly CI is set up |
|
@mpharrigan - I think it would be better to apply the skip mark at the test function definition rather than in its body. |
|
These are parameterized tests using the "outer product" over the two parameter sets. Most of the parameter settings are not slow; just the cases identified by the |
|
Yes, I add a skip for the same tests in quantumlib/Cirq#6211. |
|
Got it, cool! |
Make it easier to skip slow tests.
Test is already skipped at its parametrization.
Done in 66af330...d783c51 |
| "data", [[[1, 2, 3, 4, 5]], [[1, 2, 3], [4, 5, 10]], [[1], [2], [3], [4], [5], [6]]] | ||
| "data,num_controls", | ||
| [ | ||
| pytest.param( |
There was a problem hiding this comment.
this is a really nice thing to know about, thanks @pavoljuhas !
qualtran/bloqs/qrom_test.py
Outdated
| if num_controls == 2 and len(data) == 6: | ||
| pytest.skip("slow") |
There was a problem hiding this comment.
I think this should be removed now, right?
There was a problem hiding this comment.
oh I see two of them you skip in the decorator and this one you mark as slow and skip in the body. Is this to show how to do it when we set up a "slow" marker?
There was a problem hiding this comment.
|
The problem is we don't actually use or respect the slow marker |
There is no `slow` mark setup for pytest yet.
Yes, thank you for catching that. I overlooked the mark after a copy-paste from Cirq. We can setup a |
|
I opened #491 to track |
xref #114
This is about 47 seconds worth of tests