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

Fix stderr not printing in TestGoGenerateVendoredPackages #206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samherrmann
Copy link
Contributor

Before this commit, the TestGoGenerateVendoredPackages test failed to print stderr to the terminal as expected. Stderr was not printed because the test expected ExitError.Stderr to be populated. Per documentation ExitError.Stderr is only populated by the Cmd.Output method, which the test does not use. With this commit, the test uses a buffer to record stderr and include it in the failed test message as previously intended.

This issue was discovered because the TestGoGenerateVendoredPackages test expects moq to be available in the PATH. That means that running tests (go test ./...) in this project without first installing moq (go install) results in the TestGoGenerateVendoredPackages test to fail.

Before this commit, the error message looked as follows:

--- FAIL: TestGoGenerateVendoredPackages (0.03s)
    moq_test.go:635: Wait: exit status 1

This error message does not include the output from stderr, making it difficult to know why the test is failing.

With this commit, the error message looks as follows:

--- FAIL: TestGoGenerateVendoredPackages (0.03s)
    moq_test.go:641: Wait: exit status 1 user/user.go:5: running
    "moq": exec: "moq": executable file not found in $PATH

Before this commit, the TestGoGenerateVendoredPackages test failed to
print stderr to the terminal as expected. Stderr was not printed because
the test expected ExitError.Stderr to be populated. Per documentation
[ExitError.Stderr] is only populated by the [Cmd.Output] method,
which the test does not use. With this commit, the test uses a buffer to
record stderr and include it in the failed test message as previously
intended.

This issue was discovered because the TestGoGenerateVendoredPackages
test expects moq to be available in the PATH. That means that running
tests (go test ./...) in this project without first installing moq (go
install) results in the TestGoGenerateVendoredPackages test to fail.

Before this commit, the error message looked as follows:

    --- FAIL: TestGoGenerateVendoredPackages (0.03s)
        moq_test.go:635: Wait: exit status 1

This error message does not include the output from stderr, making it
difficult to know why the test is failing.

With this commit, the error message looks as follows:

    --- FAIL: TestGoGenerateVendoredPackages (0.03s)
        moq_test.go:641: Wait: exit status 1 user/user.go:5: running
        "moq": exec: "moq": executable file not found in $PATH

[ExitError.Stderr]: https://pkg.go.dev/os/exec#ExitError
[Cmd.Output]: https://pkg.go.dev/os/exec#Cmd.Output
@samherrmann
Copy link
Contributor Author

While the changes in this pull request fix the intended behavior (as described in the commit message), I do wonder what the goal of the TestGoGenerateVendoredPackages test is. From what I could see, it is the only test that expects moq to be available in the PATH. Could we not change TestGoGenerateVendoredPackages such that go install is not a prerequisite to running go test ./...?

@samherrmann samherrmann marked this pull request as ready for review August 31, 2023 19:37
err = cmd.Wait()
if err != nil {
if exitErr, ok := err.(*exec.ExitError); ok {
t.Errorf("Wait: %s %s", exitErr, string(exitErr.Stderr))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As described in the commit message, because this test does not use cmd.Output(), exitErr.Stderr is always empty.

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.

1 participant