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.

Sign up for GitHub

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

Update depguard rules to prevent importing test code outside tests #51001

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Update depguard rules to prevent importing test code outside tests #51001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 117 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,123 @@ linters-settings:
desc: 'use "log/slog" instead'
- pkg: golang.org/x/exp/slog
desc: 'use "log/slog" instead'
# Prevent importing testify in production code.
testify:
files:
# Tests can import testify
- '!$test'
# Exceptions
# Remove these once they are complaint.
- '!**/api/testhelpers/**'
- '!**/e/lib/auth/ssotestlib.go'
- '!**/e/lib/aws/identitycenter/test/**'
- '!**/e/lib/idp/saml/testenv/**'
- '!**/e/lib/operatortest/**'
- '!**/e/tests/**'
- '!**/lib/automaticupgrades/basichttp/servermock.go'
- '!**/lib/auth/helpers.go'
- '!**/lib/auth/keystore/testhelpers.go'
- '!**/lib/auth/test/**'
- '!**/lib/backend/test/**'
- '!**/lib/events/athena/test.go'
- '!**/lib/events/test/**'
- '!**/lib/kube/proxy/utils_testing.go'
- '!**/lib/services/suite/**'
- '!**/lib/srv/mock.go'
- '!**/lib/srv/db/redis/test.go'
- '!**/lib/teleterm/gatewaytest/**'
- '!**/lib/utils/testhelpers.go'
- '!**/lib/utils/testutils/**'
- '!**/integration/appaccess/fixtures.go'
Copy link
Contributor

Choose a reason for hiding this comment

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

A few comments/notes:

  1. Could integration/ and integrations/ get a full pass?
  2. Dedicated packages for test helpers are a more robust solution to standalone files, as these are likely to be fully excluded from the complete binary.
  3. We could use a testonly build tag for some of these, which is a stronger guarantee than depguard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could integration/ and integrations/ get a full pass?

Maybe integration/ but integrations are not tests, they are our plugins, and are imported directly into teleport.e.

these are likely to be fully excluded from the complete binary

I would not rely on Go's dead code removal to guarantee this.

We could use a testonly build tag for some of these

I thought about this but chasing tags in all the various ways we seem to run tests in CI seemed like a more involved task. This for now at least puts a halt to things until we can evaluate other options, including a build tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not rely on Go's dead code removal to guarantee this.

I think it does work on a per-package basis. A strings check for testenv does come up empty.

I thought about this but chasing tags in all the various ways we seem to run tests in CI seemed like a more involved task. This for now at least puts a halt to things until we can evaluate other options, including a build tag.

Yep, it's a bigger task for sure. Just throwing some ideas out there.

- '!**/integration/appaccess/jwt.go'
- '!**/integration/appaccess/pack.go'
- '!**/integration/db/fixture.go'
- '!**/integration/hsm/helpers.go'
- '!**/integration/helpers/**'
- '!**/integration/proxy/proxy_helpers.go'
- '!**/integrations/access/email/testlib/**'
- '!**/integrations/access/datadog/testlib/**'
- '!**/integrations/access/discord/testlib/**'
- '!**/integrations/access/jira/testlib/**'
- '!**/integrations/access/mattermost/testlib/**'
- '!**/integrations/access/msteams/testlib/**'
- '!**/integrations/access/opsgenie/testlib/**'
- '!**/integrations/access/pagerduty/testlib/**'
- '!**/integrations/access/servicenow/testlib/**'
- '!**/integrations/access/slack/testlib/**'
- '!**/integrations/lib/testing/integration/accessrequestsuite.go'
- '!**/integrations/lib/testing/integration/app.go'
- '!**/integrations/lib/testing/integration/authhelper.go'
- '!**/integrations/lib/testing/integration/suite.go'
- '!**/integrations/operator/controllers/resources/testlib/**'
- '!**/tool/teleport/testenv/**'
deny:
- pkg: github.com/stretchr/testify
desc: 'testify should not be imported outside of test code'
# Prevent importing integration test helpers in production code.
integration:
files:
# Tests can do anything
- '!$test'
- '!**/integration/**'
- '!**/e/tests/**'
- '!**/integrations/operator/controllers/resources/testlib/**'
deny:
- pkg: github.com/gravitational/teleport/integration
desc: 'integration test should not be imported outside of intergation tests'
allow:
# integrations is explicitly allowed becuase the deny rule above
# will match both integration and integrations, however only
# integration should be denied.
- github.com/gravitational/teleport/integrations
list-mode: lax
# Prevent importing testing in production code.
testing:
files:
# Tests can do anything
- '!$test'
# Exceptions
# Remove these once they are complaint.
- '!**/api/testhelpers/**'
- '!**/e/lib/auth/ssotestlib.go'
- '!**/e/lib/aws/identitycenter/test/**'
- '!**/e/lib/devicetrust/testenv/**'
- '!**/e/lib/devicetrust/storage/storage.go'
- '!**/e/lib/idp/saml/testenv/**'
- '!**/e/lib/jamf/testenv/**'
- '!**/e/lib/okta/api/oktaapitest/**'
- '!**/e/lib/operatortest/**'
- '!**/e/tests/**'
- '!**/integration/**'
- '!**/integrations/access/email/testlib/**'
- '!**/integrations/access/msteams/testlib/**'
- '!**/integrations/access/slack/testlib/**'
- '!**/integrations/operator/controllers/resources/testlib/**'
- '!**/lib/auth/helpers.go'
- '!**/lib/auth/keystore/testhelpers.go'
- '!**/lib/auth/test/**'
- '!**/lib/automaticupgrades/basichttp/servermock.go'
- '!**/lib/backend/test/**'
- '!**/lib/cryptosuites/precompute.go'
- '!**/lib/cryptosuites/internal/rsa/rsa.go'
- '!**/lib/events/test/**'
- '!**/lib/events/athena/test.go'
- '!**/lib/fixtures/**'
- '!**/lib/kube/proxy/utils_testing.go'
- '!**/lib/modules/test.go'
- '!**/lib/service/service.go'
- '!**/lib/services/local/users.go'
- '!**/lib/services/suite/**'
- '!**/lib/srv/mock.go'
- '!**/lib/srv/db/redis/test.go'
- '!**/lib/teleterm/gatewaytest/**'
- '!**/lib/utils/cli.go'
- '!**/lib/utils/testhelpers.go'
- '!**/lib/utils/testutils/**'
- '!**/tool/teleport/testenv/**'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a depguard rule that only allows "testenv" imports for tests and other testenv packages? I think these are in the clear now, but might as well add the rule here.

deny:
- pkg: testing
desc: 'testing should not be imported outside of tests'
# Prevent importing internal packages in client tools or packages containing
# common interfaces consumed by them that are known to bloat binaries or break builds
# because they only support a single platform.
Expand Down
Loading