-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
Doctests do not remove built files if they fail #3389 #3655
Conversation
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.
Thanks for the fix, @shubhambhar007! Looks good, one small suggestion.
a32beb5
to
da51644
Compare
@agriyakhetarpal i think i force pushed on top of your commit , didnt check the thread😅 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3655 +/- ##
========================================
Coverage 99.59% 99.59%
========================================
Files 258 258
Lines 20797 20798 +1
========================================
+ Hits 20712 20713 +1
Misses 85 85 ☔ View full report in Codecov by Sentry. |
I think it was the same suggestion that I updated your branch with, we should be fine – thanks! Looks like the missing coverage is unrelated. |
Yes ,i thought so too, the subrpocess call is being fed arguments by us itself and is not subject to any outside code injection, should be fine in my opinion :D . |
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.
Looks Good, thanks @shubhambhar007
Example notebook tests failing doesn't look like a cause of this PR, should we re-run jobs? |
I did so just now, it seems that #3492 broke something even though it shouldn't have |
Description
As pointed by @agriyakhetarpal , in case of failure of doctests, built files (inside the docs/build/ folder) are not deleted.
We also have to ensure deletion in case of successful doctests. I added a finally block for the same and put the subprocess call in a try block.
Fixes #3389
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: