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

Skip tests that require network access with HERMETIC=true #587

Merged
merged 2 commits into from
May 17, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented May 16, 2022

The config tests make outbound network calls since Load() calls
prepare() which fetches the OIDC configs. Tests that call Load() will
now be skipped. Tested with go test -tags=hermetic ./... without a
network connection.

Signed-off-by: Hayden Blauzvern hblauzvern@google.com

Summary

Ticket Link

Fixes #480

Release Note

In hermetic environments, tests that require network access can be skipped with -tags=hermetic

The config tests make outbound network calls since Load() calls
prepare() which fetches the OIDC configs. Tests that call Load() will
now be skipped. Tested with `HERMETIC=true go test ./...` without a
network connection.

I didn't use go:build !hermetic beacuse this causes the build to skip
building the config package entirely.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

cc @06kellyjac

06kellyjac
06kellyjac previously approved these changes May 16, 2022
Copy link

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

looking forward to this addition. Thanks :)

@nsmith5
Copy link
Contributor

nsmith5 commented May 16, 2022

Two nits:

  • I think hermetic tests should be the default and testing with additional requirements should be opt-in
  • I prefer build tags here so that e.g go test ./... -tags=networking enables the tests that require networking.

@haydentherapper
Copy link
Contributor Author

I'd prefer to continue running all tests by default. If we forget to enable the network tests for some reason, we'll lose some coverage. If we get more reports of tests run in hermetic environments, then we can consider switching, but since GitHub Actions runners have network access, I'm not too concerned.

//go:build !hermetic caused a build issue, pkg/config wasn't built so go test ./... failed - If I understand this correctly, the tests would need to be in their own build package with a negative statement.

@nsmith5
Copy link
Contributor

nsmith5 commented May 16, 2022

You just need to move the non-hermetic tests to a seperate file like this: nsmith5@c4703f9

Can confirm that this diff works as expected (e.g go test runs all tests and go test -tags=hermetic only runs the hermetic ones)

@06kellyjac
Copy link

if default is hermetic and non-hermetic is opt in I think the tag should be nonhermetic or networked or something

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

Yep, splitting it up resolved the issue. This switched it to go test -tags=hermetic ./... to skip tests that require network access. We'll leave the default to be running all tests.

@dlorenc dlorenc merged commit a1b85d4 into sigstore:main May 17, 2022
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.

GetIssuer just panics rather than issuing a useful error when there's no networking
4 participants