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 dataPowered 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 dataPowered 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?