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

x/telemetry/godev/cmd/telemetrygodev: test fails when run in a standalone module #65258

Open
bcmills opened this issue Jan 24, 2024 · 12 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Jan 24, 2024

Go version

6e7aee5

Output of go env in your module/workspace:

N/A

What did you do?

https://build.golang.org/log/a1fcb0cfb4ce3595205204cea723c2d7a5c447e5

What did you see happen?

2024/01/22 21:17:02 open ../../../config/config.json: no such file or directory
FAIL	golang.org/x/telemetry/godev/cmd/telemetrygodev	0.278s

Note that the android builder runs tests by copying the entire module containing the package to the builder, similar to running go test on the package from outside of its module.

The replace directive added in https://go.dev/cl/499918 causes the golang.org/x/telemetry/godev module to depend on an invalid version of golang.org/x/telemetry, but if you also go get a valid version of that module, the failure is also reproducible on any platform by running the test from the module cache:

$ go version
go version devel go1.23-b4e7d630 Wed Jan 24 07:25:25 2024 +0000 linux/amd64

$ go mod init example
go: creating new go.mod: module example

$ go get -t golang.org/x/telemetry/godev/cmd/telemetrygodev@master golang.org/x/telemetry@master
go: downloading golang.org/x/telemetry v0.0.0-20240123174154-00d1ee814984
go: downloading golang.org/x/telemetry/godev v0.0.0-20240123174154-00d1ee814984
go: downloading github.com/yuin/goldmark v1.5.4
go: downloading github.com/yuin/goldmark-meta v1.1.0
go: downloading golang.org/x/mod v0.14.0
go: downloading golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63
go: downloading cloud.google.com/go/storage v1.30.1
go: downloading google.golang.org/api v0.149.0
go: downloading gopkg.in/yaml.v2 v2.4.0
go: downloading cloud.google.com/go v0.110.8
go: downloading google.golang.org/grpc v1.59.0
go: downloading cloud.google.com/go/compute/metadata v0.2.3
go: downloading github.com/google/uuid v1.4.0
go: downloading cloud.google.com/go/iam v1.1.3
go: downloading golang.org/x/oauth2 v0.13.0
go: downloading github.com/googleapis/gax-go/v2 v2.12.0
go: downloading google.golang.org/genproto v0.0.0-20231016165738-49dd2c1f3d0b
go: downloading google.golang.org/protobuf v1.31.0
go: downloading cloud.google.com/go/compute v1.23.1
go: downloading google.golang.org/genproto/googleapis/api v0.0.0-20231016165738-49dd2c1f3d0b
go: downloading go.opencensus.io v0.24.0
go: downloading golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2
go: downloading google.golang.org/genproto/googleapis/rpc v0.0.0-20231016165738-49dd2c1f3d0b
go: downloading google.golang.org/appengine v1.6.7
go: downloading github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da
go: downloading golang.org/x/net v0.17.0
go: downloading github.com/golang/protobuf v1.5.3
go: downloading golang.org/x/sys v0.16.0
go: downloading golang.org/x/text v0.13.0
go: downloading github.com/googleapis/enterprise-certificate-proxy v0.3.2
go: downloading github.com/google/s2a-go v0.1.7
go: downloading golang.org/x/sync v0.4.0
go: downloading golang.org/x/crypto v0.14.0
go: added cloud.google.com/go v0.110.8
go: added cloud.google.com/go/compute v1.23.1
go: added cloud.google.com/go/compute/metadata v0.2.3
go: added cloud.google.com/go/iam v1.1.3
go: added cloud.google.com/go/storage v1.30.1
go: added github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da
go: added github.com/golang/protobuf v1.5.3
go: added github.com/google/s2a-go v0.1.7
go: added github.com/google/uuid v1.4.0
go: added github.com/googleapis/enterprise-certificate-proxy v0.3.2
go: added github.com/googleapis/gax-go/v2 v2.12.0
go: added github.com/yuin/goldmark v1.5.4
go: added github.com/yuin/goldmark-meta v1.1.0
go: added go.opencensus.io v0.24.0
go: added golang.org/x/crypto v0.14.0
go: added golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63
go: added golang.org/x/mod v0.14.0
go: added golang.org/x/net v0.17.0
go: added golang.org/x/oauth2 v0.13.0
go: added golang.org/x/sync v0.4.0
go: added golang.org/x/sys v0.16.0
go: added golang.org/x/telemetry v0.0.0-20240123174154-00d1ee814984
go: added golang.org/x/telemetry/godev v0.0.0-20240123174154-00d1ee814984
go: added golang.org/x/text v0.13.0
go: added golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2
go: added google.golang.org/api v0.149.0
go: added google.golang.org/appengine v1.6.7
go: added google.golang.org/genproto v0.0.0-20231016165738-49dd2c1f3d0b
go: added google.golang.org/genproto/googleapis/api v0.0.0-20231016165738-49dd2c1f3d0b
go: added google.golang.org/genproto/googleapis/rpc v0.0.0-20231016165738-49dd2c1f3d0b
go: added google.golang.org/grpc v1.59.0
go: added google.golang.org/protobuf v1.31.0
go: added gopkg.in/yaml.v2 v2.4.0

$ go test golang.org/x/telemetry/godev/cmd/telemetrygodev
2024/01/24 09:46:05 open ../../../config/config.json: no such file or directory
FAIL    golang.org/x/telemetry/godev/cmd/telemetrygodev 0.090s
FAIL

Per #34352 (comment) the test failure should also be reproducible on the LUCI builders, but for some reason it is not — no failures are showing up on https://ci.chromium.org/p/golang/g/x-telemetry-gotip/console. I will follow up on that separately.

What did you expect to see?

All tests passing, including when run from within the module cache.

@gopherbot gopherbot added the telemetry x/telemetry issues label Jan 24, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jan 24, 2024
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed telemetry x/telemetry issues labels Jan 24, 2024
@bcmills
Copy link
Contributor Author

bcmills commented Jan 24, 2024

Reopened #34352 for the missing LUCI builder coverage.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 24, 2024

The test failure was introduced in https://go.dev/cl/556860 (attn @findleyr @hyangah).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/557839 mentions this issue: godev/cmd/telemetrygodev: fix TestPaths when run from the module cache

@hyangah
Copy link
Contributor

hyangah commented Jan 24, 2024

The module is not intended to run as a standalone module. It is tightly coupled with the x/telemetry module and the intention is to test the three modules in the repo are consistent.

There is also no reason to run any tests in this module on android. I think the right fix is to disable the tests in unintended platform completely.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 24, 2024

If it is tightly coupled with x/telemetry, would it make sense to combine the two modules back into one so that it doesn't start failing once #34352 is fixed?

(What is the value of keeping the nested godev module separate?)

@hyangah
Copy link
Contributor

hyangah commented Jan 24, 2024

If it is tightly coupled with x/telemetry, would it make sense to combine the two modules back into one so that it doesn't start failing once #34352 is fixed?

The go toolchains will depend on x/telemetry (and uploader pulls into x/telemetry/config) and we don't want all the dependencies of x/telemetry/godev pulled into the go command.

x/telemetry/config and x/telemetry/godev are built and deployed together.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 24, 2024

Module graph pruning should prevent the dependencies of x/telemetry/godev from being pulled up to the cmd module.

We already rely on that, for example, to prune out the dependency on rsc.io/pdf from the golang.org/x/arch module.

@hyangah
Copy link
Contributor

hyangah commented Jan 24, 2024

Is there a reason that we need to run telemetry.go.dev on android?

This is a result of a long discussion to keep the x/telemetry module minimal and small. (no typescript or media files in the module people need to import).

@findleyr
Copy link
Contributor

FWIW, I don't think it matters in this test (see the trivial fix), but agree that we don't need to test the godev sub module on anything but linux. However, this is probably unnecessarily complicated to configure on the builders: we definitely want to run x/telemetry module tests on all platforms.

We can skip non-target platforms in our tests of the godev module as necessary. But it's not necessary in this case.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 24, 2024

I am more concerned about the impact on fixing #34352 than about running on Android in particular.

An exemption to allow x/telemetry tests to fail when run from the module cache would add yet another special case to consider in maintaining the builders going forward, and we have already been having trouble keeping up with builder maintenance for quite some time.

For comparison, consider golang.org/x/tools/internal/testenv.NeedsLocalXTools, which skips cross-module tests in x/tools where appropriate.

@hyangah
Copy link
Contributor

hyangah commented Jan 24, 2024

As our project portfolio grows, I think we need to reevaluate the requirements of #34352. Not all Go modules our team is working on are intended to be hosting packages and tools that can be gettable or installed on by external users.

For this specific issue, I think we can just fix the tests.

golang.org/x/telemetry/internal/testenv has various conditions. We can add a condition there too.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/563358 mentions this issue: godev/cmd/telemetrygodev: skip tests in unintended environments

gopherbot pushed a commit to golang/telemetry that referenced this issue Feb 13, 2024
golang.org/x/telemetry/godev is a module that hosts code for the
telemetry.go.dev service. This module is not intended to work
standalone, but assumes the whole telemetry repo is cloned.
Running this service on platforms like android, ios, etc is not
our focus either. The production service is likely running on linux,
but we also want to be able to test, develop on darwin or windows.

* Skip platforms that are not likely used by x/telemetry/godev
contributors. e.g.  android, ios, ...
* Skip if 'go' is not available.
* Run go list to find x/telemetry module. When go list runs from
a directory under x/telemetry/godev, the command will return the
replaced directory path, which is also the repo root.

For golang/go#65549
For golang/go#65258

Change-Id: I88f5f51bd0f2c355975127e1fa9559394e307f97
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/563358
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

4 participants