asv icon indicating copy to clipboard operation
asv copied to clipboard

Add `finalize_teardown` method to clean up if setup or teardown fail

Open Alphare opened this issue 5 years ago • 11 comments

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.

Alphare avatar Jul 23 '19 08:07 Alphare

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?

Alphare avatar Jul 23 '19 08:07 Alphare

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.

pv avatar Jul 23 '19 10:07 pv

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. :)

Alphare avatar Jul 23 '19 10:07 Alphare

I've remove the "raise or not" mechanism, which fixes the issue of multiple fix_setup not being able to run if one raises.

Alphare avatar Jul 23 '19 10:07 Alphare

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.

pv avatar Jul 23 '19 14:07 pv

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.

Alphare avatar Jul 23 '19 15:07 Alphare

Codecov Report

Merging #858 into master will increase coverage by 0.04%. The diff coverage is 20%.

Impacted file tree graph

@@            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[bot] avatar Jul 25 '19 07:07 codecov[bot]

Codecov Report

Merging #858 into master will increase coverage by 0.04%. The diff coverage is 20%.

Impacted file tree graph

@@            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[bot] avatar Jul 25 '19 07:07 codecov[bot]

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 avatar Sep 04 '19 08:09 Alphare

@Alphare could you give maintainers access to modify the PR? I'd like to rebase and clean this up.

HaoZeke avatar May 21 '23 00:05 HaoZeke

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

Alphare avatar May 23 '23 08:05 Alphare