-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Improve path handling in run-tests #5877
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
Conversation
@@ -57,7 +65,7 @@ def run_micropython(pyb, args, test_file, is_special=False): | |||
had_crash = False | |||
if pyb is None: | |||
# run on PC | |||
if test_file.startswith(('cmdline/', 'feature_check/')) or test_file in special_tests: | |||
if test_file.startswith(('cmdline/', base_path('feature_check/'))) or test_file in special_tests: |
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.
Why does feature_check
need base_path
but cmdline
doesn't?
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.
Because test_file will still be a relative path in most cases, except not for feature_check tests. In main()
tests still get listed relative to the test directory, also see my comment on this PR above to why that is, and I added this to the hlpe text as well.
tests/run-tests
Outdated
BASEPATH = os.path.dirname(os.path.abspath(inspect.getsourcefile(lambda: None))) | ||
|
||
def base_path(*p): | ||
return os.path.join(BASEPATH, *p).replace('\\', '/') |
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.
Would it be safer/better to return and absolute/canonical path here?
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.
You mean to strip out ..
etc? Yes that's probably better
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.
yes, that is what I mean
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.
Ok changed that.
Nice. We could also update |
That won't work, same reason as above. I mean it could work but requires quite a lot of changes. |
2773a62
to
7fbc9f3
Compare
Would it be less work to make the |
In comparison with this version that wouldn't change much, would just mean adding code to |
@stinos thanks for the patch. Have you been using this in the meantime? Do you still think it's a good approach? |
Sorry for the late reply. I've been using this unmodified since creating the patch, but that's actually only a couple of 'fixed' usecases i.e. I just changed 2 or 3 places where tests get ran to not cd into the tests directory. I think the approach is ok, also because I don't see a better way at the moment, though in hindsight I'd maybe mimize the diff by not removing the |
Do you mean, not adding the |
dd396ea
to
e336bc3
Compare
Depends on how you look at it; I update the code so that run_feature_check still takes a base_path as argument so all calls to it can remain unchanged. And the commit messages are reworded a bit. |
This has the same CI error running |
See #6374 for a proposed fix to the uasyncio_fair test. |
Replace some usages of paths relative to the current working directory with absolute paths relative to the tests directory. Fixes and resulting changes: - default values of MICROPYTHON and MPYCROSS are absolute paths and always correct - likewise, the correct full paths for tools and extmod directories are appended to sys.path - printing/cleaning failures works properly since it expects the .exp and .out files in the tests directory which is also where they are written to now, plus no more need for changing directories This fixes micropython#5872 and allows running custom tests which use run-tests without having to cd to the tests directory first, and the test output still is in the tests/ directory instead of the current working directory. Discovery of tests and all skip test logic based on paths relative to the current working directory remains unchanged which essentially means that for running most of MicroPython's own tests, run-tests must still be ran from within it's directory, so document that.
A configurable result directory is advantageous because it enables using a dedicated location, eventually outside of the source tree, instead of forcing the output files into a fixed directory which might also contain other files already. For that reason the default output directory also has been changed to tests/results/.
e336bc3
to
df7c9d4
Compare
Great, thanks. I do like these changes, and it's much better to have all the .exp/.out files now go in a Also thanks for clean commit messages, this was merged as-is (although needed to be rebased). |
Another benefit of these changes is that it's now possible to run multiple instances of the test suite (eg testing 2 different boards at the same time) and the results can go to different directories. |
Add softdev_version 7.0.1
A fix for #5872: basically use full paths instead of current working directory for everything except test discovery and skip_tests logic. That would require a lot of extra changes, results in full paths being printed in the pass/fail output, results in cmdline and other tests failing because they have the relative cmdline path baked into their .exp file and so on.
In the end all this practically does is: