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

Make Junit report success callout message conditional #13154

Closed

Conversation

Rajalakshmi-Girish
Copy link
Contributor

@Rajalakshmi-Girish Rajalakshmi-Girish commented Jun 28, 2021

gotestsum error messages like ./scripts/test_lib.sh: line 240: gotestsum: command not found go unnoticed.
In spite of this error message, the current logic at https://github.com/etcd-io/etcd/blob/main/scripts/test_lib.sh#L245 misguides that the Junit report is saved.

Hence this PR is to add the neccessary addition of $GOPATH/bin to $PATH and making the log_callout message conditional.

@Rajalakshmi-Girish
Copy link
Contributor Author

/cc @serathius @ptabor

@Rajalakshmi-Girish
Copy link
Contributor Author

/recheck

@Rajalakshmi-Girish Rajalakshmi-Girish force-pushed the junit__test_report branch 2 times, most recently from 2e41fbf to 286c334 Compare June 28, 2021 20:04
@serathius
Copy link
Member

serathius commented Jun 29, 2021

I think we should instead use other way to install it. Current way of installing gotestsum is not used in other places, we use gobin like 8a5733c.

@Rajalakshmi-Girish
Copy link
Contributor Author

I think we should instead use other way to install it. Current way of installing gotestsum is not used in other places, we use gobin like 8a5733c.

yes that seems the right way to install gotestsum tool.
Shall I correct it in this PR else shall we wait for #13152 to get merged?

@serathius
Copy link
Member

I think best would be to close this PR to avoid conflicts. I'm still finding small problems with this script, for example with cross compilation, so I would prefer to do a thorough testing before merging.

@Rajalakshmi-Girish
Copy link
Contributor Author

#13152 shall take care of this.

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

Successfully merging this pull request may close these issues.

2 participants