Add finalize_teardown method to clean up if setup or teardown fail#858
Add finalize_teardown method to clean up if setup or teardown fail#858Alphare wants to merge 1 commit intoairspeed-velocity:mainfrom
finalize_teardown method to clean up if setup or teardown fail#858Conversation
|
Maybe it would be a better idea to allow this method to return a new exception to be raised, because I'm not certain of the use case of silencing the exception... or maybe do both? |
|
Why not use |
|
This is easier if I have multiple setup methods, or when using inheritence. I'd say it's arguably also uglier. My real sweet spot would be something akin to |
|
I've remove the "raise or not" mechanism, which fixes the issue of multiple |
|
I see. That the setup/teardown routines are not paired is a design problem. If the fixture-type behavior is wanted, how about instead a |
|
Sure, this was a bit of a placeholder name, I didn't want to waste time on that considering we were probably going to make changes in this PR. This |
Codecov Report
@@ Coverage Diff @@
## master #858 +/- ##
==========================================
+ Coverage 70.97% 71.01% +0.04%
==========================================
Files 95 95
Lines 12843 12848 +5
Branches 2103 2106 +3
==========================================
+ Hits 9115 9124 +9
- Misses 3328 3329 +1
+ Partials 400 395 -5
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #858 +/- ##
==========================================
+ Coverage 70.97% 71.01% +0.04%
==========================================
Files 95 95
Lines 12843 12848 +5
Branches 2103 2106 +3
==========================================
+ Hits 9115 9124 +9
- Misses 3328 3329 +1
+ Partials 400 395 -5
Continue to review full report at Codecov.
|
Alphare
changed the title
fix_setup method to clean up if setup failsfinalize_teardown method to clean up if setup or teardown fail
pv
left a comment
pv
left a comment
There was a problem hiding this comment.
I guess this should run in finally:? Also, should have test.
asv/benchmark.py
Outdated
| benchmark.do_profile(profile_path) | ||
| finally: | ||
| benchmark.do_teardown() | ||
| except BaseException as e: |
There was a problem hiding this comment.
Using finally would mean resorting to using sys.exc_info() to get the exception info and checking if it differs from (None, None, None), which to me feels more convoluted than just using an except clause.
There was a problem hiding this comment.
Yes, but the point is that the finalize should run always, not only for exceptions.
If the `setup` or `teardown` methods fail, the tests session waits for timeout (1800s per benchmark!) provided a sub process had been created beforehand. This lead to excruciatingly long test sessions, appearing fully stuck.
Alphare
force-pushed
the
setup-fallback
branch
from
0d7700e to
6c492e0
Compare
|
Note: I am aware of the CI failures but need to figure #871 out before attempting to fix the tests since it would be very time consuming and spammy otherwise to just force-push every time. |
pv
added
the
needs-work
|
@Alphare could you give maintainers access to modify the PR? I'd like to rebase and clean this up. |
|
@HaoZeke I would be happy to, I just don't know how to do it once a PR has already been created. It may just be easier to close this one and for you to re-open one with the same changes? |
3 participants
If the
setupmethod fails, the tests session waits for timeout (1800s perbenchmark!) provided a sub process had been created beforehand. This lead to
excruciatingly long test sessions, appearing fully stuck.