-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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 slow test methods with @requires_resource('cpu') #108416
Comments
Isn't it the same as #108388 ? |
No, they are different. The purpose is different, this issue is about speeding up manual testing, although it can help in CI testing too. I only mark the slowest methods, allowing fast tests to run. #108388 is much larger and more complex issue, this issue only intersects with a small part of it.. |
This doesn't feel right. Tagging these as 'cpu' is usually untrue. Many of these tests are not resource intensive for anything but 'wall clock time'. Critically, it would means we have no easy reliable way of running this vast swath of our testsuite across multiple platforms when doing development if our Github CI does not use whatever resource is being applied so that the tests become skipped by default. If a dev has to do something special to identify and make tests potentially relevant change run across all platforms, they simply not going to get that determination right or remember a large portion of the time and the tests will not be run until after merge when it is too late. Allowing regressions to be merged is a net negative. Tangentially related: We should probably have all github actions CI configurations in release branches set to use |
This comment was marked as resolved.
This comment was marked as resolved.
I think we want a new resource class for this purpose when latency is the main reason it is being disabled rather than actual CPU consumption. I suggest perhaps calling it We could make a new Even better if it is possible for the github merge queue automerge to wait for that not otherwise required CI check even though the merge button blocker does not? I don't know if github allows that level of config... @ambv ? (brainstorming) |
Re-did using I prepared also a separate patch for marking long running tests which do not spend much CPU time, but simply sleep, with a new resource "walltime". |
…es_resource('walltime')
…108421) Only mark tests which spend significant system or user time, by itself or in subprocesses.
pythonGH-108421) Only mark tests which spend significant system or user time, by itself or in subprocesses. (cherry picked from commit f3ba0a7) Co-authored-by: Serhiy Storchaka <[email protected]>
…e('cpu') (pythonGH-108421) Only mark tests which spend significant system or user time, by itself or in subprocesses.. (cherry picked from commit f3ba0a7) Co-authored-by: Serhiy Storchaka <[email protected]>
See also #108828 which will help to find all tests which can be skipped for particular reason. |
…ource('walltime') (GH-108480)
…es_resource('walltime') (pythonGH-108480) (cherry picked from commit 1e0d627) Co-authored-by: Serhiy Storchaka <[email protected]>
… requires_resource('walltime') (pythonGH-108480). (cherry picked from commit 1e0d627) Co-authored-by: Serhiy Storchaka <[email protected]>
…es_resource('walltime') (pythonGH-108480) (cherry picked from commit 1e0d627)
* gh-108834: regrtest reruns failed tests in subprocesses (#108839) When using --rerun option, regrtest now re-runs failed tests in verbose mode in fresh worker processes to have more deterministic behavior. So it can write its final report even if a test killed a worker progress. Add --fail-rerun option to regrtest: exit with non-zero exit code if a test failed pass passed when re-run in verbose mode (in a fresh process). That's now more useful since tests can pass when re-run in a fresh worker progress, whereas they failed when run after other tests when tests are run sequentially. Rename --verbose2 option (-w) to --rerun. Keep --verbose2 as a deprecated alias. Changes: * Fix and enhance statistics in regrtest summary. Add "(filtered)" when --match and/or --ignore options are used. * Add RunTests class. * Add TestResult.get_rerun_match_tests() method * Rewrite code to serialize/deserialize worker arguments as JSON using a new WorkerJob class. * Fix stats when a test is run with --forever --rerun. * If failed test names cannot be parsed, log a warning and don't filter tests. * test_regrtest.test_rerun_success() now uses a marker file, since the test is re-run in a separated process. * Add tests on normalize_test_name() function. * Add test_success() and test_skip() tests to test_regrtest. (cherry picked from commit 31c2945) * gh-108834: regrtest --fail-rerun exits with code 5 (#108896) When the --fail-rerun option is used and a test fails and then pass, regrtest now uses exit code 5 ("rerun) instead of 2 ("bad test"). (cherry picked from commit 1170d5a) * gh-108416: Mark slow but not CPU bound test methods with requires_resource('walltime') (GH-108480) (cherry picked from commit 1e0d627) * Manually sync Lib/test/libregrtest/ from main --------- Co-authored-by: Serhiy Storchaka <[email protected]>
Has this already been discussed elsewhere?
This is a minor feature, which does not need previous discussion elsewhere
Links to previous discussion of this feature:
No response
Proposal:
The proposed PR marks all test methods which have duration longer than 3 seconds with
@test.support.requires_resource('cpu')
decorator.The purpose is to reduce manual testing time. It happens that all tests in a file are ran in fraction of a second, but few tests take a long time to run. When you work on some large feature or bugfix you need to run corresponding tests multiple times. You can exclude the slowest tests manually, but you should know what of them are culprits. When they are marked as CPU-hungry, you can just not enable the "cpu" resource.
For example, all
test_math
takes over 1.5 minutes to finish. But when excludetest_sumprod_stress
, it takes only 3 seconds.Linked PRs
The text was updated successfully, but these errors were encountered: