Skip to content
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.

Sign up for GitHub

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

Jump to bottom

dist: move testrunner to pythonlibs as package #9759

Merged
merged 4 commits into from
Aug 16, 2018
Merged

dist: move testrunner to pythonlibs as package #9759

merged 4 commits into from
Aug 16, 2018

Conversation

smlng
Copy link
Member

@smlng smlng commented Aug 10, 2018

Contribution description

As testrunner is moved to dist/pythonlibs which is exported
via PYTHONPATH, testrunner is found by all test scripts.

Issues/PRs references

#9758

@smlng smlng added Area: tests Area: tests and testing framework State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Aug 10, 2018
@smlng smlng requested a review from cladmi August 10, 2018 10:20
@smlng smlng force-pushed the pr/pythonlibs/testrunner branch from 03d153a to 693d413 Compare August 10, 2018 11:42
@aabadie
Copy link
Contributor

aabadie commented Aug 10, 2018

Nice one. I like it.

aabadie
aabadie reviewed Aug 10, 2018
View reviewed changes
tests/bench_msg_pingpong/tests/01-run.py
@@ -16,6 +15,5 @@ def testfunc(child):


if __name__ == "__main__":
sys.path.append(os.path.join(os.environ['RIOTTOOLS'], 'testrunner'))
Copy link
Contributor

Choose a reason for hiding this comment

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

The line below can now be moved with the other imports at the top of the script.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes will do

@smlng
Copy link
Member Author

smlng commented Aug 10, 2018

@aabadie please note this is based on #9758, maybe you can also have a look at that one, too ... THX!

@aabadie
Copy link
Contributor

aabadie commented Aug 10, 2018

@smlng, I don't think removing sys is a good idea. See this

@smlng
Copy link
Member Author

smlng commented Aug 10, 2018

I don't think removing sys I

was suggested by @cladmi, but reading the linked blog-entry it sound reasonable to use sys.exit in out context - so should I drop the change?

@smlng smlng force-pushed the pr/pythonlibs/testrunner branch from 7a42d3e to 782e163 Compare August 10, 2018 14:44
@smlng smlng removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 10, 2018
@smlng smlng force-pushed the pr/pythonlibs/testrunner branch from 782e163 to 5f286f6 Compare August 10, 2018 14:45
@cladmi
Copy link
Contributor

cladmi commented Aug 11, 2018

was suggested by @cladmi, but reading the linked blog-entry it sound reasonable to use sys.exit in out context - so should I drop the change?

My bad, I did not know about this https://docs.python.org/3/library/constants.html#exit and never had issues before so was wondering why.

@smlng
Copy link
Member Author

smlng commented Aug 13, 2018

dropped the removal of import sys and fixed flake8 errors reported by Murdock, good to review this one again - needs squashing now. @cladmi and @aabadie?

aabadie
aabadie approved these changes Aug 13, 2018
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK, please squash!

smlng added 4 commits August 13, 2018 14:11
@smlng
dist: move testrunner to pythonlibs as package
dd9a43b 
As testrunner is moved to dist/pythonlibs which is exported
via PYTHONPATH, testrunner is found by all test scripts.
@smlng
tests: use testrunner from pythonlibs
13d61b5 
Remove now obsolete sys.append from all tests, as testrunner was moved
to dist/pythonlibs as proper package.
@smlng
tests: remove unused os package import
f9824ab 
os package is imported by every test script but only used by
a few, thus flake8 check reported errors.
@smlng
tests: move testrunner import up
3893f04 
Testrunner is now impported as a package found in PYTHONPATH, so
import can be placed at the top of the script as usual.
@smlng smlng force-pushed the pr/pythonlibs/testrunner branch from 1631293 to 3893f04 Compare August 13, 2018 12:11
@smlng
Copy link
Member Author

smlng commented Aug 13, 2018

squashed, I'd still like to have an ACK by @cladmi on this one, too. So please don't merge right away, but leave him do it, thx.

@cladmi
Copy link
Contributor

cladmi commented Aug 13, 2018

I will take care of reviewing after the release.

@cladmi
Copy link
Contributor

cladmi commented Aug 14, 2018

Changes look good, I will re-run tests and see if something complains.

cladmi
cladmi approved these changes Aug 16, 2018
View reviewed changes
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

I could re-run tests on iotlab-m3.

@cladmi cladmi merged commit 31aba49 into RIOT-OS:master Aug 16, 2018
@cladmi
Copy link
Contributor

cladmi commented Aug 16, 2018

Some gardening may need to be done after PRs still using sys.append.path are merged.
The tests may be found using:

git grep sys.path.append

@smlng smlng deleted the pr/pythonlibs/testrunner branch August 16, 2018 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cladmi cladmi cladmi approved these changes

@aabadie aabadie aabadie approved these changes

Assignees
No one assigned
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smlng @aabadie @cladmi