Coverage test sys.settrace & improve coverage #17538
Conversation
|
Code size report: |
|
While there are some other errors going on that also leave me scratching my head, the "expected error" happens on Windows CI, specifically "build-vs (x64, Debug, dev, 2017)". |
|
I can't come up with any good theory as to what's making those unix builds fail. It's also a surprise that it's only ONE of the msvc x64 builds. |
|
While working on extending sys.settrace() with additional capabilities I also noticed that the debug builds are not tested across ports. I first noticed Errors first popping up when building an ESP32 locally. Is there a way to determine coverage locally? |
d134cdb to
c4d5f09
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17538 +/- ##
==========================================
- Coverage 98.56% 98.23% -0.34%
==========================================
Files 169 171 +2
Lines 21948 22140 +192
==========================================
+ Hits 21634 21749 +115
- Misses 314 391 +77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Yes, you can go through the steps of the unix coverage build locally. In gcov files show coverage on a line by line basis, with "#####" indicating no coverage of the line, "-" indicating no coverage possible, and a number indicating the number of times the line was covered. The format is described in |
|
The nanbox build were failing like this: The reason is that a |
|
This PR is now failing only because it decreases the overall coverage percentage. In reality, it adds coverage to 21751 previously not covered lines. |
|
I've updated the PR so that settrace is only enabled in the coverage build, not the standard build or other variants. we should consider whether this makes the standard+settrace CI build redundant. I can remove that if desired. the standard+stackless+settrace CI job is probably still useful. |
Yes, that's a good idea. As mentioned, it really decreases performance. OTOH, it's important to get coverage of the settrace code, so enabling it on the coverage build makes sense.
Yes, we can now simplify those CI jobs. These settrace jobs were added in 0b85b5b . Maybe we can replace those two with just a test for stackless mode? Or at least just remove the standard+settrace CI and keep standard+stackless+settrace (as you suggest). |
|
wrt the work on
That makes it simpler to run that subset during development.
|
That sounds reasonable, although not in scope for this PR.
This PR will enable settrace on the unix coverage build, so once this PR is merged you could use that. |
|
@jepler there's a failing test here on the CI which I think is due to the code taking longer to run now that settrace is enabled. It looks like the combination of already-long-running-test + settrace + ubsan = tests now takes longer than the 30s timeout. I guess the simplest way to fix this is just increase |
I'll increase the timeout globally if that's ok. Or I could pipe through a timeout multiplier used only for 1 build. |
Might be worth adding a command line argument for this timeout? And then increase it just for this build? Because if it's increased globally, eg to 60 seconds, then that's a long time to wait to know that a small test failed, if it locks up. |
|
I added an environment variable override and set it from ci.sh when running coverage tests. I tested that locally, by making sure that a timeout of 2 produced a handful of failures. |
Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
This side-steps the problem where the output does not match when the host python version is 3.12.x (as it is on unix ci today). Signed-off-by: Jeff Epler <jepler@gmail.com>
If the fields added for MICROPY_PY_SYS_SETTRACE are not initialized properly, their value in a thread is indeterminate. In particular, if the callback is not NULL, it will be invoked as a function. Signed-off-by: Jeff Epler <jepler@gmail.com>
When MICROPY_PY_SYS_SETTRACE was enabled, a crash was seen in the qemu_mips build. It seems likely that this was due to these added fields not being initialized. Signed-off-by: Jeff Epler <jepler@gmail.com>
The argument corresponding to a `%q` specifier must be of type `qstr`, not a narrower type like `int16_t`. Not ensuring this caused an assertion error on one Windows x64 build. The argument corresponding to a `%d` specifier must be of type `int`, not a potentially-wider type like `mp_uint_t`. Not ensuring this prevented the function name from being printed on the unix nanbox build. Signed-off-by: Jeff Epler <jepler@gmail.com>
This becomes redundant when the main coverage build includes settrace. Signed-off-by: Jeff Epler <jepler@gmail.com>
3fd7cea to
b3d1321
Compare
The additional overhead of the settrace profiler means that the `aes_stress.py` test was running too slowly on GitHub CI. Double the timeout to 60 seconds. Signed-off-by: Jeff Epler <jepler@gmail.com>
This removes the need for the `sys_settrace_features.py.exp` file. This means that people testing locally will also need to install Python 3.11 in some way, such as with pyenv or uv, and use it during `make VARIANT=coverage test`, or they will get failures. When using python from GitHub actions/setup-python, pip3 can't be wrapped by sudo, because this invokes the operating system python instead. Signed-off-by: Jeff Epler <jepler@gmail.com>
as requested in code review. Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
|
Let me know when this is ready. |
|
Thank you! |
Summary
I noticed that
py/profile.cwas not covered at all according to codecov, due to the fact it was never enabled in the unix port.I enabled it, and then added a new test to cover some previously un-covered lines.
Testing
I ran the tests locally and reviewed the coverage of profile.c.
Note: I suspect one or more Windows x64 builds will fail due to improved coverage in this PR, exposing a latent problem.
I wrote a rambling post in the discussions area about passing qstrs to mp_printf. The TL;DR explanation is, there appears to be undefined behavior that occurs when a "small qstr" (
uint16_t) argument such astype->nameis printed via the%qformat specifier, which retrieves assize_tvalue. The specific behavior I saw with x64 msvc on godbolt looked like it would trigger on one specific format string in the core, the repr of frame objects. That's what spurred me to ensure that this code was covered.If the failure does occur, I'll try to write it up properly as an issue.