-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
test: Improve use of test fixtures in integration tests #6773
Conversation
Marking this as a draft to ensure we don't merge it prior to #6770 going in. The substantive diff is 1095f05...dfae402 |
I've merged in the ci branch from #6649 to see if it runs these tests happily or not. |
@@ -71,3 +72,71 @@ jobs: | |||
flags: unittests # optional | |||
fail_ci_if_error: true # optional (default = false) | |||
verbose: true # optional (default = false) | |||
|
|||
|
|||
Integration: |
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 might need some adjustment, I will try to push them this week After my Eid vacation ends
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.
No rush, this won't go anywhere until after 5.1 anyway
This pull request introduces 1 alert and fixes 1 when merging 7ff79f6 into 2411504 - view on LGTM.com new alerts:
fixed alerts:
|
@@ -1,5 +1,4 @@ | |||
pytest-cov | |||
pytest-travis-fold |
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.
Good idea.
@thedrow - those diffs are from #6649 rather than my own changes. Probably best to transplant those comments over there for @auvipy / whoever picks that works up. My diff is just 1095f05...dfae402. Don't stress too much about this PR for the moment, I'll re-request review when it's cleaner and has a working base for integration tests in CI. |
This fixes some issues with worker shutdown taking too long when subsets of the tests are run. There's probably a better way to do this but at least this stops the errors.
These wouldn't be run because the class name was bad. Fixing that exposed some breakage in the tests themselves which needed to be fixed.
Co-authored-by: Omer Katz <omer.drow@gmail.com>
0f38b75
to
64ee882
Compare
This has been rebased on top of current master but still merges in an old version of the |
Codecov Report
@@ Coverage Diff @@
## master #6773 +/- ##
==========================================
+ Coverage 70.73% 75.66% +4.93%
==========================================
Files 138 138
Lines 16605 16605
Branches 2094 2094
==========================================
+ Hits 11745 12565 +820
+ Misses 4663 3824 -839
- Partials 197 216 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This pull request introduces 1 alert when merging 64ee882 into 5d72aee - view on LGTM.com new alerts:
|
Description
Test suite improvements based on the time I spent in the canvas tests for #6770 . This
changeset is based on that PR but could be separated if necessary - TBH I think it's
easiest to just block merge of this on that one.