Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

stinos
Copy link
Contributor

@stinos stinos commented Apr 7, 2020

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:

  • using a fixed and configurable path for the test output
  • make it more convenient to use run-tests for testing custom code (which is something I do a lot actually, very handy to just compare against expected CPython output or a .exp file with regexes) by not requiring to cd into the tests directory and correctly deriving tools/extmod/MICROPYTHON/MPYCROSS paths

@@ -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:
Copy link
Contributor

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?

Copy link
Contributor Author

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('\\', '/')
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok changed that.

@dlech
Copy link
Contributor

dlech commented Apr 7, 2020

Nice. We could also update .travis.yml and makefiles to no longer cd when running tests.

@stinos
Copy link
Contributor Author

stinos commented Apr 7, 2020

That won't work, same reason as above. I mean it could work but requires quite a lot of changes.

@stinos stinos force-pushed the run-tests-abs-path branch from 2773a62 to 7fbc9f3 Compare April 8, 2020 06:04
@dlech
Copy link
Contributor

dlech commented Apr 8, 2020

Would it be less work to make the run-tests change the current working directory to the tests/ directory when it is run? Any paths passed as command line arguments or environment variables should be treated as relative to the current directory from where the script was launched of course. They would need to be resolved to absolute paths before changing the working directory. But then after that, the reset of the script should not need to be changed.

@stinos
Copy link
Contributor Author

stinos commented Apr 8, 2020

Would it be less work to make the run-tests change the current working directory

In comparison with this version that wouldn't change much, would just mean adding code to cd and a finally clause to cd back. And that would then allow to not having to manually cd to the tests directory. But would also be superfluous for custom tests.

@dpgeorge dpgeorge added the tests Relates to tests/ directory in source label Apr 9, 2020
@dpgeorge
Copy link
Member

@stinos thanks for the patch. Have you been using this in the meantime? Do you still think it's a good approach?

@stinos
Copy link
Contributor Author

stinos commented Jul 6, 2020

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 base_path argument from the feature_check function.

@dpgeorge
Copy link
Member

dpgeorge commented Jul 8, 2020

though in hindsight I'd maybe mimize the diff by not removing the base_path argument from the feature_check function.

Do you mean, not adding the base_path argument to run_feature_check(...)? If so then yes +1. Can you make such a change to this PR?

@stinos stinos force-pushed the run-tests-abs-path branch 2 times, most recently from dd396ea to e336bc3 Compare August 25, 2020 10:24
@stinos
Copy link
Contributor Author

stinos commented Aug 25, 2020

Do you mean, not adding the base_path argument to run_feature_check(...)

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.

@dpgeorge
Copy link
Member

This has the same CI error running uasyncio_fair.py.

@dpgeorge
Copy link
Member

See #6374 for a proposed fix to the uasyncio_fair test.

stinos added 2 commits August 26, 2020 11:53
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/.
@stinos stinos force-pushed the run-tests-abs-path branch from e336bc3 to df7c9d4 Compare August 26, 2020 09:53
@dpgeorge
Copy link
Member

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.

Great, thanks. I do like these changes, and it's much better to have all the .exp/.out files now go in a results/ directory instead of the directory that run-tests is in.

Also thanks for clean commit messages, this was merged as-is (although needed to be rebased).

@dpgeorge dpgeorge closed this Aug 27, 2020
@dpgeorge
Copy link
Member

Merged in 405893a and 0c3f9d5

@stinos stinos deleted the run-tests-abs-path branch August 27, 2020 07:09
@dpgeorge
Copy link
Member

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.

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Relates to tests/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants