Skip to content
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

Add finalize_teardown method to clean up if setup or teardown fail #858

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Alphare
Copy link
Contributor

@Alphare Alphare commented Jul 23, 2019

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
Copy link
Contributor Author

Alphare commented Jul 23, 2019

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?

@pv
Copy link
Collaborator

pv commented Jul 23, 2019

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.

@Alphare
Copy link
Contributor Author

Alphare commented Jul 23, 2019

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
Copy link
Contributor Author

Alphare commented Jul 23, 2019

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

@pv
Copy link
Collaborator

pv commented Jul 23, 2019

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.

@Alphare
Copy link
Contributor Author

Alphare commented Jul 23, 2019

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
Copy link

codecov bot commented Jul 25, 2019

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%) ⬇️
asv/commands/preview.py 52.45% <0%> (-3.28%) ⬇️
asv/runner.py 91.37% <0%> (+1.32%) ⬆️
test/test_util.py 92.97% <0%> (+2.16%) ⬆️

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.

1 similar comment
@codecov
Copy link

codecov bot commented Jul 25, 2019

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%) ⬇️
asv/commands/preview.py 52.45% <0%> (-3.28%) ⬇️
asv/runner.py 91.37% <0%> (+1.32%) ⬆️
test/test_util.py 92.97% <0%> (+2.16%) ⬆️

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.

@Alphare Alphare changed the title Add fix_setup method to clean up if setup fails Add finalize_teardown method to clean up if setup or teardown fail Jul 25, 2019
Copy link
Collaborator

@pv pv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally: I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Alphare commented Sep 4, 2019

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 pv added the needs-work The PR cannot be merged as is, further work required (explained in comments) label Nov 24, 2019
@HaoZeke
Copy link
Member

HaoZeke commented May 21, 2023

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

@Alphare
Copy link
Contributor Author

Alphare commented May 23, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-work The PR cannot be merged as is, further work required (explained in comments)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants