tests: Fix more tests failing with terse error messages.#19324
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19324 +/- ##
=======================================
Coverage 98.47% 98.47%
=======================================
Files 176 176
Lines 22845 22845
=======================================
Hits 22497 22497
Misses 348 348 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| function ci_unix_standard_terse_run_tests { | ||
| MICROPY_TEST_TIMEOUT=60 make -C ports/unix VARIANT=standard test |
There was a problem hiding this comment.
I don't think the timeout needs to be increased here. Best to leave it at the default if possible.
|
Did you see 9ecdeb6 ? Maybe some of the tests here can be added to that list if they really need to check the exact form of the error string. |
|
Code size report: |
I didn't know of such a thing. Looking at the modified tests in this PR I can definitely see Out of the extra tests that fail on |
|
I should say that the general direction of this PR is good: adding CI to test terse error message builds, and fixing related things up. Just need to tune it to get it passing all the existing CI. |
|
Thanks, I've updated the PR to make it work. |
This commit updates `extmod/ssl_keycert`, `extmod/tls_sslcontext_ciphers`, and `extmod/vfs_blockdev_invalid` tests to also pass even if the interpreter was built with terse error messages. Originally the tests assumed the error messages level was always higher than `MICROPY_ERROR_REPORTING_TERSE`, and thus expected a particular string pattern to appear in certain errors' output. Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
| error_reporting_tests_to_skip["terse"] = error_reporting_tests_to_skip["none"] | ||
| error_reporting_tests_to_skip["terse"] = error_reporting_tests_to_skip["none"] + ( | ||
| "cmdline/repl_paste.py", | ||
| "misc/sys_settrace_features.py", |
There was a problem hiding this comment.
These probably need to be skipped when error reporting is "none", so could just go in the tuple above?
There was a problem hiding this comment.
That's true, but on the other hand there's no build testing things with said error reporting level, so adding things to terse should still be reasonable in practice.
In the meantime I'll still put those in the none list. I guess I'll need to have a go at building unix/standard with that reporting level to see which tests need to be added to the list then. That'll be another PR.
Edit: well, it doesn't even build. Building the Unix port with make -j8 VARIANT=standard CFLAGS_EXTRA="-DMICROPY_ERROR_REPORTING=MICROPY_ERROR_REPORTING_NONE" MICROPY_ROM_TEXT_COMPRESSION=0 yields this:
...
In file included from ../../py/emitnx64.c:16:
../../py/emitnative.c:207:13: error: ‘vtype_to_qstr’ defined but not used [-Werror=unused-function]
207 | static qstr vtype_to_qstr(vtype_kind_t vtype) {
| ^~~~~~~~~~~~~
...
../../py/nativeglue.c:323:5: error: ‘mp_raise_msg’ undeclared here (not in a function)
323 | mp_raise_msg,
| ^~~~~~~~~~~~
../../py/nativeglue.c:323:5: note: ‘mp_raise_msg’ is a function-like macro and might be used incorrectly
Fixing the build errors makes tests/feature_check/target_info.py fail too :(
Anyway, things build and run with this:
diff --git i/py/emitnative.c w/py/emitnative.c
index 6cf01dcab..781aacf74 100644
--- i/py/emitnative.c
+++ w/py/emitnative.c
@@ -204,6 +204,7 @@ typedef enum {
VTYPE_BUILTIN_CAST = 0x70 | MP_NATIVE_TYPE_OBJ,
} vtype_kind_t;
+#if MICROPY_ERROR_REPORTING != MICROPY_ERROR_REPORTING_NONE
static qstr vtype_to_qstr(vtype_kind_t vtype) {
switch (vtype) {
case VTYPE_PYOBJ:
@@ -227,6 +228,7 @@ static qstr vtype_to_qstr(vtype_kind_t vtype) {
return MP_QSTR_None;
}
}
+#endif
typedef struct _stack_info_t {
vtype_kind_t vtype;
diff --git i/py/nativeglue.c w/py/nativeglue.c
index e4aa635cf..e9164ee4c 100644
--- i/py/nativeglue.c
+++ w/py/nativeglue.c
@@ -320,7 +320,11 @@ const mp_fun_table_t mp_fun_table = {
gc_realloc,
mp_printf,
mp_vprintf,
+ #if MICROPY_ERROR_REPORTING == MICROPY_ERROR_REPORTING_NONE
+ NULL,
+ #else
mp_raise_msg,
+ #endif
mp_obj_get_type,
mp_obj_new_str,
mp_obj_new_bytes,
diff --git i/tests/feature_check/target_info.py w/tests/feature_check/target_info.py
index ed91b27b7..35bb6b4ad 100644
--- i/tests/feature_check/target_info.py
+++ w/tests/feature_check/target_info.py
@@ -40,6 +40,6 @@ except NameError:
try:
(lambda: 0)(0)
except TypeError as er:
- error_reporting = {0: "none", 27: "terse", 54: "normal", 56: "detailed"}[len(er.value)]
+ error_reporting = {0: "none", 27: "terse", 54: "normal", 56: "detailed"}[len(er.value or '')]
print(platform, arch, arch_flags, build, thread, float_prec, len("α") == 1, error_reporting)and the list of failing tests is this: basics/deque_micropython.py, basics/generator_pend_throw.py, basics/int_64_basics.py, basics/subclass_native_exc_new.py, extmod/ssl_keycert.py, extmod/ssl_keycert_pkcs8.py, extmod/vfs_blockdev_invalid.py, float/math_fun.py, float/math_fun_special.py, extmod/cryptolib_aes128_ctr.py, extmod/ssl_cadata.py, micropython/import_mpy_invalid.py, micropython/import_mpy_native.py, micropython/viper_error.py, extmod/asyncio_cancel_self.py, extmod/tls_sslcontext_ciphers.py, stress/bytecode_limit.py, stress/qstr_limit.py, stress/qstr_limit_str_modulo.py, extmod/asyncio_gather_notimpl.py. PR coming up soon.
This commit updates the list of tests to skip if the error message level is less or equal to "terse", adding `cmdline/repl_paste` and `misc/sys_settrace_features`. For the former, in situations where space is at a premium usually the REPL isn't enabled. If it is, changes to its internals are usually covered by other tests. The latter, on the other hand, would require either a fundamentally reworked test script, or modifying the test harness to cater for two very different outputs set for a single test. Using regular expressions here won't help, as the changes involve block of lines rather than parts of a single line at a time. Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
This commit adds a new CI target for the Unix port, which is the `standard` variant being built with terse error messages. Even though it won't cover all possible tests failing with terse error messages (some occur only in the `coverage` build variant), this should be a good start for preventing new tests to not assume a particular error message level. To make things automated, the new target is also added to the regular CI jobs set in the Unix port's GitHub workflow file. This closes micropython#17707. Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
Octoprobe PR report
FailuresGroup: run-tests.py
Group: run-tests.py --via-mpy --emit native
|
Summary
This PR fixes all tests appearing in the Unix port's
standardvariant when built with terse error messages.As mentioned in #17707, there still are tests faiing with this specific configuration. The commits in this PR attempt to address this situation and add a new CI job to routinely test the Unix port to make sure new tests won't depend on strings that may change depending on the error reporting level.
There are still some tests that fail with terse messages, but they only occur on the
coveragevariant. Fixing them is something I'll have in another PR since they involve messing with C code as half of those failures are inports/unix/coverage.c. Fixing the Python ones wouldn't really help CI-wise until the native ones are updated anyway.This fixes #17707.
Testing
This is going to be tested by the new CI job, added as part of this PR.
Trade-offs and Alternatives
Since this adds a new CI job to the list, CI runs may get a tiny bit slower. The Unix/standard job usually takes around 30 seconds or so to run so its impact shouldn't really be felt during regular development.
Generative AI
I did not use generative AI tools when creating this PR.