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

Jump to bottom

x/sys/unix: mkmerge_test.go is not tested on the Go builders #49484

Closed
bcmills opened this issue Nov 9, 2021 · 7 comments
Closed

x/sys/unix: mkmerge_test.go is not tested on the Go builders #49484

bcmills opened this issue Nov 9, 2021 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 9, 2021

unix/mkmerge.go was updated in CL 296889 to add //go:build lines (#41184).

That caused the test in unix/mkmerge_test.go to start failing, but nobody noticed because mkmerge_test.go itself has the ignore constraint — it must be run explicitly to run it at all, and the Go builders do not know to do that.

Probably the simplest solution is to factor mkmerge.go out into a proper package (perhaps x/sys/unix/internal/mkmerge?), in which case its test will be run automatically by the builders.

Another option would be to add a Test function in x/sys/unix itself that runs go test mkmerge.go mkmerge_test.go as a subprocess.

CC @tklauser @jupj @ianlancetaylor
@gopherbot gopherbot added this to the Unreleased milestone Nov 9, 2021
@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. labels Nov 9, 2021
@bcmills bcmills modified the milestones: Unreleased, Backlog Nov 9, 2021
@jupj
Copy link
Contributor

jupj commented Nov 10, 2021

Factoring mkmerge.go out into a proper package sounds like a good and simple solution, testing is transparent in that way IMHO.

Do we need to verify that the Go builders catch the failing tests in mkmerge_test.go, before fixing the tests?

In this case the process could be:

  1. First patch set: factor mkmerge.go and mkmerge_test.go into x/sys/unix/internal/mkmerge
  2. See that the Go builders fail for mkmerge_test.go
  3. Follow-up patch set (or separate CL?): fix the tests in mkmerge_test.go and see that Go builders do not fail anymore.

@bcmills
Copy link
Contributor Author

bcmills commented Nov 10, 2021

I already fixed the failing test, so we don't need to observe it on the builder. Just factoring out a package ought to suffice.

If you want to see it in action, I would suggest publishing a patchset to Gerrit with an intentional regression in the test, running TryBots on that Gerrit change to confirm that the regression is caught, and then publishing a second patchset that removes the regression.

@gopherbot
Copy link

Change https://golang.org/cl/363334 mentions this issue: x/sys/unix: factor out mkmerge into a package

@jupj
Copy link
Contributor

jupj commented Nov 11, 2021 •
edited

I did an attempt at package x/sys/unix/internal/merge in CL 363334. When running in Linux, x/sys/unix/mkall.sh prints the following error for mkmerge:

----- GENERATING: merging generated files -----
go: go.mod file not found in current directory or any parent directory; see 'go help modules'
could not merge zerrors files: exit status 1
***** FAILURE:    merging generated files *****

The Docker-based build system for Linux uses a volume mounted at x/sys/unix, which doesn't include x/sys/go.mod, hence package internal/mkmerge is not part of any module when used within the Docker-based build system.

Thoughts on how to address this?

@bcmills
Copy link
Contributor Author

bcmills commented Nov 11, 2021

Oh, neato! We should probably change the build system to mount x/sys instead of x/sys/unix, since x/sys/go.mod is what declares the version of the language against which the package is written.

@gopherbot
Copy link

Change https://golang.org/cl/363594 mentions this issue: unix: mount Docker-based builder at x/sys

@jupj
Copy link
Contributor

jupj commented Nov 12, 2021

Thanks for the input!

I sent CL 363594 to change the build system to mount x/sys. Package internal/mkmerge seems to work as intended now.

gopherbot pushed a commit to golang/sys that referenced this issue Nov 12, 2021
@jupj @tklauser
unix: mount Docker-based builder at x/sys
7c45f0c 
Mount the data volume for the Docker-based builder at x/sys instead of
x/sys/unix.

The x/sys/go.mod file was not included in the data volume when mounting
x/sys/unix. This breaks module-aware go commands that run within the
Docker container.

Fix this problem by mounting the volume at x/sys and updating the
working directory of the builder accordingly.

Updates golang/go#49484

Change-Id: I2346b5320413b48de4984c9d9e31203941336357
Reviewed-on: https://go-review.googlesource.com/c/sys/+/363594
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Trust: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Nov 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@jupj @bcmills @gopherbot