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

testscript: import timeout behavior from stdlib #171

Merged
merged 1 commit into from
Jan 13, 2023
Merged

testscript: import timeout behavior from stdlib #171

merged 1 commit into from
Jan 13, 2023

Conversation

FiloSottile
Copy link
Contributor

This uses the -test.timeout flag (or the Params.Deadline field) to send first SIGQUIT (to get a stack trace) and then SIGKILL to a stuck command. It's mostly imported from the current stdlib code, with tweaks to work around the lack of Deadline and Cleanup method access.

@mvdan
Copy link
Collaborator

mvdan commented Jul 28, 2022

Did you somehow write this PR on top of an older master? GitHub reports conflicts :)

@FiloSottile
Copy link
Contributor Author

Ah yep, I did, I've been using this for a bit in my fork. I'll rebase.

Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Requesting changes as a reminder that this needs a rebase.

@FiloSottile
Copy link
Contributor Author

Conflict resolved! :)

@mvdan
Copy link
Collaborator

mvdan commented Jan 13, 2023

There are conflicts again, my apologies. We recently merged a regression in the -continue flag, and I forgot that this was in flight already. If you can fix them again, which should hopefully not be too hard, I'll make sure to review quickly.

This uses the -test.timeout flag (or the Params.Deadline field) to send
first SIGQUIT (to get a stack trace) and then SIGKILL to a stuck
command.
@FiloSottile
Copy link
Contributor Author

Done!

Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Thanks! For some context, this started appearing upstream in https://go-review.googlesource.com/c/go/+/233526.

@mvdan mvdan merged commit 9957a52 into rogpeppe:master Jan 13, 2023
@myitcv
Copy link
Collaborator

myitcv commented Feb 10, 2024

@FiloSottile - out of interest, why did this line get added as part of this change?

https://github.com/rogpeppe/go-internal/blame/2c88e7f58ae1ca6f811b818d0d985b4622556532/testscript/testscript.go#L443

It appears unrelated to the main purpose of the PR, but would like to check whether I'm missing something in that analysis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants