-
Notifications
You must be signed in to change notification settings - Fork 963
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
Update tests to use sysmon #16621
Update tests to use sysmon #16621
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @DarkaMaul! I can confirm that this significantly speeds up the test suite in local runs for me.
I suspect coverage.py's branch performance regressions with sys.monitoring
are pretty workload-dependent -- Warehouse is possibly just lucky enough to have a workload/layout that isn't causing significant issues relative to the baseline gains of using lightweight monitoring 🙂
Definitely cool speedup! Looks like this has some impact on how contexts are measured?
We measure contexts here: Line 3 in 6ce90e9
This provides visual indicators in the HTML report to show which test functions are hitting a given line of code, making it easier to figure out which tests are calling what. Is there anything on coverage.py tracking that issue, so we can leave ourselves a link to check in on it later on? |
I think we should simply monitor This PR can be either put on hold until coverage changes, or we can use the new setting for CI runs (with the possibility for interested developers to use it locally too). |
I think we'd likely benefit from the speedups where possible. Can you provide a hook to allow the developer to pass along a flag to revert the behavior? I don't think the current patch allows setting the envvar that passes to docker compose command in the Makefile, so giving the end user some pay of passing down that the COVERAGE_CORE should revert to to |
Coverage introduced in Coverage.py 7.4.0 the support for
sys.monitoring
.While it should not work well with the
branch
coverage used within warehouse, tests on our computers with @woodruffw showed a significant improvement:Tests results
sysmon
:26.7s
29.36s
56.38s
50.08s
The relevant issue in coverage is nedbat/coveragepy#1812
We would like to gather more feedback before eventually pushing this option forward.
NOTE: The other tracers are
ctrace
orpytrace
/cc @woodruffw