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

Teach Test() to abort test if ctrl-C is pressed #2900

Merged
merged 1 commit into from
Oct 8, 2018

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Oct 5, 2018

This is a bit of a hack, but seems to work very well in practice. You can even resume the test with return if you want (but the interrupted line will still lead to diffs).

An arguably "better" (but much more complicated) solution would be to add a flag which the kernel uses to signal to the error handler that the error is a user interrupt; then we could support seamlessly resuming of the interrupted test. But this requires further work, e.g. re-redirecting input/output temporarily, saving and restoring BreakOnError and more. Not sure that this would be worth it. In the meantime, this simple solution already gets us most of the benefit.

Resolves #1879

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: tests issues or PRs related to tests topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Oct 5, 2018
@codecov
Copy link

codecov bot commented Oct 5, 2018

Codecov Report

Merging #2900 into master will decrease coverage by <.01%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master    #2900      +/-   ##
==========================================
- Coverage   83.73%   83.72%   -0.01%     
==========================================
  Files         680      680              
  Lines      346376   346636     +260     
==========================================
+ Hits       290046   290234     +188     
- Misses      56330    56402      +72
Impacted Files Coverage Δ
lib/test.gi 64.31% <40%> (-0.28%) ⬇️
src/hpc/c_type1.c 83.12% <0%> (-1.29%) ⬇️
src/objects.c 80.53% <0%> (-0.69%) ⬇️
src/c_type1.c 84.04% <0%> (-0.64%) ⬇️
src/objset.c 84.49% <0%> (-0.23%) ⬇️
src/compiler.c 88.13% <0%> (-0.03%) ⬇️
src/weakptr.c 99.2% <0%> (ø) ⬆️
lib/read4.g 100% <0%> (ø) ⬆️
src/hpc/tls.h 66.66% <0%> (ø) ⬆️
src/precord.c 94.88% <0%> (ø) ⬆️
... and 8 more

Copy link
Contributor

@ChrisJefferson ChrisJefferson left a comment

Choose a reason for hiding this comment

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

Could you add a brief comment that explains this code. I know it's in the git commit, but I feel this is subtle enough that it should be in the code.

@fingolfin
Copy link
Member Author

Good point; comment added

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

That's a very handy feature! Been there hitting ctrl-C a dozen times trying to terminate the test. I've tested this with entering return, quit and QUIT in the break loop, all work as expected.

@ChrisJefferson
Copy link
Contributor

I've got one more comment. Should we check the string starts with Error, user interrupt? searching through the source, there are things like "user interrupt in sleep" and "user interrupt while printing". Do we want to catch those too?

This is a bit of a hack, but seems to work very well in practice.
You can even resume the test with `return` if you want (but the
interrupted line will still lead to diffs).

Resolves gap-system#1879
@fingolfin
Copy link
Member Author

@ChrisJefferson changed to use StartsWith(res, "Error, user interrupt") as test.

Copy link
Contributor

@stevelinton stevelinton left a comment

Choose a reason for hiding this comment

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

Looks good to me. If we were going to do something lass hacky it would make sense to solve the bigger problem of understanding why any code execution did not complete and being able to deal with it.

@fingolfin fingolfin merged commit 8af543f into gap-system:master Oct 8, 2018
@fingolfin fingolfin deleted the mh/abort-test branch October 8, 2018 10:19
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve interaction between ctrl-c and Test()
4 participants