-
Notifications
You must be signed in to change notification settings - Fork 160
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
Conversation
Codecov Report
@@ 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
|
There was a problem hiding this 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.
3ba2c06
to
1fe8c6c
Compare
Good point; comment added |
There was a problem hiding this 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.
I've got one more comment. Should we check the string starts with |
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
1fe8c6c
to
2cca6a7
Compare
@ChrisJefferson changed to use |
There was a problem hiding this 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.
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