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

Handle missing "pass" event for subtests #188

Closed

Conversation

aaronlehmann
Copy link
Contributor

Sometimes no "pass" event is present for subtests in the JSON output,
even though the output indicates the subtests passed (with "--- PASS:")
and the parent test had a pass event. In these cases, gotestsum can
output spurious failure messages for the subtests. Handle this by
treating a pass event for a parent test as implying that the subtests
also passed.

Sometimes no "pass" event is present for subtests in the JSON output,
even though the output indicates the subtests passed (with "--- PASS:")
and the parent test had a pass event. In these cases, gotestsum can
output spurious failure messages for the subtests. Handle this by
treating a pass event for a parent test as implying that the subtests
also passed.
p.addEndTestEvent(event, tc)
}

func (p *Package) addEndTestEvent(event TestEvent, tc TestCase) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gocyclo forced this change on me. I'm personally not a fan of gocyclo and how it can force small fixes to be bundled with refactors, but hey, it's not my codebase.

@aaronlehmann
Copy link
Contributor Author

@coryb pointed out to me that the root cause actually looks like go test misattributing certain output lines. For example, we've seen --- PASS lines with the wrong Test JSON key.

So this PR might help with the symptom, but since there appears to be a deeper issue with bad data coming from the JSON, I'm not sure it makes sense to move forward with this.

@dnephin
Copy link
Member

dnephin commented Apr 1, 2021

Hi @aaronlehmann , thank you for the PR! Yes this issue sounds familiar. I think #141 has some investigation and a link to the upstream Go issue (golang/go#38063). From that discussion it seems like the issue is more common with parallel tests.

As you found in the code there is already some logic to handle other go test or test2json bugs, so I'm not against trying to fix it in gotestsum. I'll give this PR a read over the weekend and see if we can get it merged.

Which version of Go are you using when you see these bugs? I believe the misattributing lines was a serious regression in 1.14.x , and was eventually fixed in 1.15.x. There are a few closed issues tagged with test2json-bug that document those problems.

@aaronlehmann
Copy link
Contributor Author

I'm seeing this with Go 1.14.7. I suspect my problem is related to tests writing directly to stdout/stderr. That sounds like unsupported behavior. Unfortunately it will be a bit painful to fix.

I found this upstream issue that looks related: golang/go#38050

It was ultimately fixed by tweaking the test output, rather than adjusting the parser: golang/go@2ba9d45

...so it's not a huge surprise that tests writing directly to stdout/stderr might cause the parser to get confused in a similar way.

@aaronlehmann
Copy link
Contributor Author

I believe the specific issue I'm hitting is golang/go#40771. It sounds like a fix is included in Go 1.14.10 (and 1.15.3, and 1.16). So upgrading Go will likely fix the issue for me. I'll close this since it looks like upgrading Go to get the bug fix is a better path forward.

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