RFC tests: Make settrace test work on a wider range of machines. #6690

stinos · 2020-12-10T16:44:38Z

Depending on type and version of installation the CPython output
can include 'zipimport' as well, next to the 'importlib' frames.
And on Windows the path reduction needs to deal with the different
path separator.

RFC part: this isn't completely ready yet in that the path reduction logic will still fail when running tests like

run-tests -d .\misc

because then there's no tests/misc in the path at all.
Also the current logic in sys_settrace_loop.py and sys_settrace_generator.py isn't actually needed I think, because co_filename isn't a full path in those tests.
Does this need fixing?

stinos · 2020-12-15T11:14:10Z

Tested this for more cases:

the zipimport trace change was already merged in the meantime, but the fairly common git-cmd which comes which Git for Windows and is bascially just the Windows commandline but with cmd added to the path and apparently other changes has traces including encodings, so doesn't hurt ignoring that. Can be reproduced elsewhere by setting PYTHONIOENCODING=cp1252 or similar. Though I get it might be considered too local, then the commit can be left out: in a 'standard' environment the slashes are the only thing which needs fixing
on second thought it's probably not worth fixing cases like run-tests -d .\misc because even if it gets fixed for sys_settrace_feature.py it will still fail for the other 2 tests but those have .exp files so we'd need to genarate them again

dpgeorge · 2020-12-17T11:16:54Z

tests/misc/sys_settrace_features.py

@@ -21,7 +21,7 @@ def print_stacktrace(frame, level=0):
            frame.f_globals["__name__"],
            frame.f_code.co_name,
            # reduce full path to some pseudo-relative
-            "misc" + "".join(frame.f_code.co_filename.split("tests/misc")[-1:]),
+            "misc" + "".join(frame.f_code.co_filename.replace("\\", "/").split("tests/misc")[-1:]),


how about:

"sys_settrace_" + frame.f_code.co_filename.split("sys_settrace_")[-1],

that should work for everything, even ./run-tests -d misc?

It would work for this file, but will still fail for the other 2 tests

I tried it on the other 2 and it passes (and they can be run from any dir it seems) if the .exp files are also updated.

Eg, from top-level of repo:

MICROPY_MICROPYTHON=./ports/unix/micropython-dev ./tests/run-tests -d tests/misc

(that's not a very common way to run it, I admit)

Actually I was just hesitating to change the .exp files since they are so large. But it's also bit silly to use the rather complicated "misc" + "".join(....) when it's just a matter of getting the filename out, and having the meaningless "miscmisc" in the tests.

Note what you propose still needs to replace slashes on Windows for the tests in the sys_settrace_subdir.
So: can we settle on

"sys_settrace_" + frame.f_code.co_filename.split("sys_settrace_")[-1].replace('\\', '/'),

in all 3 files, plus removal of miscmisc/ from the current .exp files?
Alternatively the slash replacement can be left out of the loop/generator tests since they are not affected.

I do often run tests like ./run-tests -d ./misc in order not to have to wait for the full test suite to complete.

Other alternative: since all our files have trace_ in the name this would work for all 3 tests:

"trace_" + frame.f_code.co_filename.split("trace_")[-1]

but sort of obfuscates the output. Which in turn could be fixed by renaming trace_generic.py to sys_settrace_generic.py and then your original proposal works again :)

Actually I was just hesitating to change the .exp files since they are so large. But it's also bit silly to use the rather complicated "misc" + "".join(....) when it's just a matter of getting the filename out, and having the meaningless "miscmisc" in the tests.

Changing the .exp files is ok, I think it's a worthy change to clean them up (the miscmisc is certainly not useful).

I think the reason the .exp's are needed is because MicroPython has different line numbers for while loops due to optimisation of the bytecode.

Note what you propose still needs to replace slashes on Windows for the tests in the sys_settrace_subdir.

Does it? Hmm... I thought that the sys_settrace_features would work b/c running CPython on the host (eg Windows) would produce the correct slash to match.

I do often run tests like ./run-tests -d ./misc in order not to have to wait for the full test suite to complete.

Yes I do that all the time, but use ./run-tests -d misc instead (which I think is still broken for these settrace tests).

So: can we settle on

"sys_settrace_" + frame.f_code.co_filename.split("sys_settrace_")[-1].replace('\\', '/'),

in all 3 files, plus removal of miscmisc/ from the current .exp files?
Alternatively the slash replacement can be left out of the loop/generator tests since they are not affected.

That sounds good to me, including leaving out the slash replacement in the loop and generator tests.

I thought that the sys_settrace_features would work b/c running CPython on the host (eg Windows) would produce the correct slash to match.

CPython will produce sys_settrace_subdir\trace_generic.py whereas MicroPython will always produce forward slashes.

(which I think is still broken for these settrace tests).

No it will work now, since the entire 'problematic' part comes before the filename and is skipped.

Just to doublecheck: did you see my previous comment with other alternatives (could have been I posted that while you were writing your answer)? The one with using "sys_settrace_" + frame.f_code.co_filename.split("sys_settrace_")[-1] everywhere and renaming the 2 files in sys_settrace_subdir is perhaps the best after all.

(which I think is still broken for these settrace tests).

No it will work now, since the entire 'problematic' part comes before the filename and is skipped.

Ah, I meant "which I think is currently/also broken for these settrace tests", but will be fixed by this PR.

Just to doublecheck: did you see my previous comment with other alternatives

Yes I did see it.

The one with using "sys_settrace_" + frame.f_code.co_filename.split("sys_settrace_")[-1] everywhere and renaming the 2 files in sys_settrace_subdir is perhaps the best after all.

If you like this approach feel free to do it that way, I'm fine with it (and it is perhaps best because there are no slashes to worry about).

Ok, took the last approach so the code remains simple and the same for all files.

dpgeorge · 2020-12-17T11:17:44Z

tests/misc/sys_settrace_features.py

    frame_name = frame.f_globals["__name__"]
-    if frame_name.find("importlib") != -1 or frame_name.find("zipimport") != -1:
+    if any(name in frame_name for name in to_ignore):


this change looks fine, shouldn't hurt

The original logic of reducing a full path to a relative one assumes "tests/misc" is in the filename which is limited in usage: it never works for CPython on windows since that will use a backslash as path separator, and also won't work when the filename is a path not relative to the tests directory which happens for example in the common case of running "./run-tests -d misc". Fix all cases by printing only the bare filename, which requires them all to start with sys_settrace_ hence the renaming.

…oding. Notably git-cmd which comes with git installations on windows alters encoding resulting in CPython tracing encodings/cp1252.py calls.

dpgeorge · 2020-12-18T02:59:28Z

Nice work, thanks! Merged in 108183f and 069557e

…n-main Translations update from Hosted Weblate

stinos force-pushed the settrace-test-fixes branch from 68ddd0a to 83c2ebc Compare December 10, 2020 16:47

stinos mentioned this pull request Dec 10, 2020

Failed 'tests/misc/sys_settrace_features.py' #6689

Closed

stinos force-pushed the settrace-test-fixes branch 2 times, most recently from becdeac to ac81c0f Compare December 15, 2020 11:04

dpgeorge added the tests Relates to tests/ directory in source label Dec 17, 2020

dpgeorge reviewed Dec 17, 2020

View reviewed changes

stinos added 2 commits December 17, 2020 15:29

tests/misc/sys_settrace_features.py: Fix running with non-default enc…

c7be332

…oding. Notably git-cmd which comes with git installations on windows alters encoding resulting in CPython tracing encodings/cp1252.py calls.

stinos force-pushed the settrace-test-fixes branch from ac81c0f to c7be332 Compare December 17, 2020 14:29

dpgeorge closed this Dec 18, 2020

stinos deleted the settrace-test-fixes branch December 18, 2020 07:55

tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 3, 2022

Merge pull request micropython#6690 from weblate/weblate-circuitpytho…

d659a3b

…n-main Translations update from Hosted Weblate

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC tests: Make settrace test work on a wider range of machines. #6690

RFC tests: Make settrace test work on a wider range of machines. #6690

stinos commented Dec 10, 2020 •

edited

Loading

stinos commented Dec 15, 2020

dpgeorge Dec 17, 2020

stinos Dec 17, 2020

dpgeorge Dec 17, 2020

stinos Dec 17, 2020

stinos Dec 17, 2020 •

edited

Loading

dpgeorge Dec 17, 2020

stinos Dec 17, 2020

dpgeorge Dec 17, 2020

stinos Dec 17, 2020 •

edited

Loading

dpgeorge Dec 17, 2020

dpgeorge commented Dec 18, 2020

RFC tests: Make settrace test work on a wider range of machines. #6690

RFC tests: Make settrace test work on a wider range of machines. #6690

Conversation

stinos commented Dec 10, 2020 • edited Loading

stinos commented Dec 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinos Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinos Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Dec 18, 2020

stinos commented Dec 10, 2020 •

edited

Loading

stinos Dec 17, 2020 •

edited

Loading

stinos Dec 17, 2020 •

edited

Loading