asv
asv copied to clipboard
Add `finalize_teardown` method to clean up if setup or teardown fail
If the setup
method fails, 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.
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 try: expect:
in your setup
instead? I'd like to avoid adding new API methods unless there's very clear need for them.
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 pytest
fixtures, but that's for another day. :)
I've remove the "raise or not" mechanism, which fixes the issue of multiple fix_setup
not being able to run if one raises.
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 finalize_teardown
hook, which runs last also when setup/call/teardown failed? If you like, it could even receive an exc_info argument for any exceptions. fix_setup
is not a descriptive name, and I'm not so sure it's really the semantics we want here.
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 finalize_teardown
method might be next best thing to real composable fixtures. I'll have a jab at it.
Codecov Report
Merging #858 into master will increase coverage by
0.04%
. The diff coverage is20%
.
@@ 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
Impacted Files | Coverage Δ | |
---|---|---|
asv/benchmark.py | 29.17% <20%> (-0.06%) |
:arrow_down: |
asv/commands/preview.py | 52.45% <0%> (-3.28%) |
:arrow_down: |
asv/runner.py | 91.37% <0%> (+1.32%) |
:arrow_up: |
test/test_util.py | 92.97% <0%> (+2.16%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update fc543c4...0d7700e. Read the comment docs.
Codecov Report
Merging #858 into master will increase coverage by
0.04%
. The diff coverage is20%
.
@@ 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
Impacted Files | Coverage Δ | |
---|---|---|
asv/benchmark.py | 29.17% <20%> (-0.06%) |
:arrow_down: |
asv/commands/preview.py | 52.45% <0%> (-3.28%) |
:arrow_down: |
asv/runner.py | 91.37% <0%> (+1.32%) |
:arrow_up: |
test/test_util.py | 92.97% <0%> (+2.16%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update fc543c4...0d7700e. Read the comment docs.
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.
@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?