Skip to content

fix: ensure the appropriate error message is produced when the RunOnFailure() option is used.#132

Open
machine424 wants to merge 1 commit intouber-go:masterfrom
machine424:fup
Open

fix: ensure the appropriate error message is produced when the RunOnFailure() option is used.#132
machine424 wants to merge 1 commit intouber-go:masterfrom
machine424:fup

Conversation

@machine424
Copy link
Copy Markdown

adjust RunOnFailure() doc.

follow up to #129

…ailure() option is used.

adjust RunOnFailure() doc.

follow up to uber-go#129
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Oct 10, 2024

CLA assistant check
All committers have signed the CLA.

Comment thread leaks.go
if opts.cleanup != nil {
return errors.New("Cleanup can only be passed to VerifyNone or VerifyTestMain")
}
if opts.runOnFailure {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This will also block its use in VerifyTestMain
I adjusted the doc to mention this has no effect on VerifyNone

keeping/adding a check in the code would require more effort/changes.

@machine424
Copy link
Copy Markdown
Author

cc @sywhang

Comment thread testmain_test.go
VerifyTestMain(dummyTestMain(7), RunOnFailure())
assert.Equal(t, 7, <-exitCode, "Exit code should not be modified")
assert.Contains(t, <-stderr, "goleak: Errors", "Find leaks on unsuccessful runs with RunOnFailure specified")
assert.Contains(t, <-stderr, "goleak: Errors on unsuccessful test run", "Find leaks on unsuccessful runs with RunOnFailure specified")
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

goleak: Errors wasn't enough as it was hiding RunOnFailure can only be passed to VerifyTestMain in this case.

@machine424
Copy link
Copy Markdown
Author

cc @sywhang I'd like to get this fix merged so we can make use of the feature.
(when you have some time)

@machine424
Copy link
Copy Markdown
Author

cc @sywhang @abhinav @r-hang
It'd be great to have a release with this fix, thanks.

@machine424
Copy link
Copy Markdown
Author

Should I understand that the repo is no longer maintained and that we should fork?
cc @sywhang @abhinav @r-hang

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.

2 participants