-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Set up tests for the extension's Python files. #4208
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
Close/open to redo the CI |
Codecov Report
@@ Coverage Diff @@
## master #4208 +/- ##
========================================
+ Coverage 58% 78% +20%
========================================
Files 363 439 +76
Lines 15612 20664 +5052
Branches 2428 3314 +886
========================================
+ Hits 8948 15958 +7010
+ Misses 6084 4701 -1383
+ Partials 580 5 -575
|
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.
This begins the path to testing our Python scripts!
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.
Please can we use pytest
. Some time ago we all decided that we'd use pytest.
Not sure whether you @ericsnowcurrently were in the meeting.
pythonFiles/tests/__main__.py
Outdated
sys.path.insert(1, DATASCIENCE_ROOT) | ||
suite = unittest.defaultTestLoader.discover(TEST_ROOT, | ||
top_level_dir=SRC_ROOT) | ||
unittest.TextTestRunner(verbosity=2).run(suite) |
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.
Just out of curiosity, why can't we just put tests in a directory and just run run the tests directly using the CLI, without having to create such files! Feels unnecessary.
Either way, pytest should take care of this.
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.
I think the point here was to simply use stdlib and not a 3rd party tool.
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.
- This was discussed with @brettcannon (@d3r3kk you too were in the meeting), and he recommended using pytest as it was the best.
-
and not a 3rd party tool.
We're already installing a tonne of 3rd party tools... E.g. for TPN, etc... i.e. no escaping installing 3rd party tools for development.
- Also, I'm not a fan of main.py magic, can't we use simple CLI for running tests, rather than hardcoding stuff in here.
I'm not saying this is wrong, but this feels too low level.
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.
Please look at the tests in tpn
- We are already using pytest
- No
__main__.py
file with initialization (lets do that via cli).
Sorry, forgot to approve all good |
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.
Sorry, forgot to approve all good
81917d4
to
651b428
Compare
(for #4033)
This is a dependency of a follow-up PR that will add a testing tool adapter for pytest.
[ ] Has a news entry file (remember to thank yourself!)[ ] Has sufficient logging.[ ] Has telemetry for enhancements.[ ] Test plan is updated as appropriate[ ]package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed)