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

cmd/testscript: add a -continue flag to not stop at the first error #189

Merged
merged 1 commit into from Nov 23, 2022

Conversation

mvdan
Copy link
Collaborator

@mvdan mvdan commented Nov 23, 2022

(see commit message)

@myitcv
Copy link
Collaborator

myitcv commented Nov 23, 2022

@mvdan I helped out in a small way by adding a test :)

Copy link
Owner

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

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

LGTM modulo one suggestion for a slight test improvement, thanks (and good catch on runT!)

cmd/testscript/testdata/continue.txt Outdated Show resolved Hide resolved
@mvdan
Copy link
Collaborator Author

mvdan commented Nov 23, 2022

I did forget a test, didn't I :)

This helps with writing and running bug reproducers using testscript.
Since they are often meant to represent the desired behavior,
and not the current behavior, they are designed to fail.

However, some reproducers consist of multiple commands,
and cmd/testscript would stop at the first command to error.
When running a reproducer which is designed to fail,
we want to run the entire script and see all failures.

Add a `-continue` flag which does just that.
With it, `T.FailNow` still marks the test as failed,
but does not stop its execution via a panic.
We also make two small changes to behave correctly when we reach the end
of a script but `T.Failed` is true; we should "FAIL" rather than "PASS".
The previous code only reached that point if no errors had happened.

Note that we also altered `runT` to use pointer method receivers.
It used atomics to modify and read its `failed` field,
but without pointer receivers, `T.Failed` always returned false.
It appears that bug had been present for a long time.

Co-Authored-By: Paul Jolly <paul@myitcv.io>
@mvdan
Copy link
Collaborator Author

mvdan commented Nov 23, 2022

Thanks! Will merge on green.

Copy link
Collaborator

@myitcv myitcv left a comment

Choose a reason for hiding this comment

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

LGTM. But I'm biased, because I helped write the test :)

@mvdan mvdan merged commit fef0545 into rogpeppe:master Nov 23, 2022
rogpeppe added a commit that referenced this pull request Jan 5, 2023
The flag was added in #189 but not documented in the usage message.
rogpeppe added a commit that referenced this pull request Jan 6, 2023
The flag was added in #189 but not documented in the usage message.
@mvdan mvdan deleted the testscript-continue branch March 27, 2023 09:36
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.

None yet

3 participants