-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Update depguard rules to prevent importing test code outside tests #51001
Conversation
e489716
to
4e0910a
Compare
d6e0845
to
fc98b57
Compare
- '!**/lib/teleterm/gatewaytest/**' | ||
- '!**/lib/utils/testhelpers.go' | ||
- '!**/lib/utils/testutils/**' | ||
- '!**/integration/appaccess/fixtures.go' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments/notes:
- Could integration/ and integrations/ get a full pass?
- 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.
- We could use a
testonly
build tag for some of these, which is a stronger guarantee than depguard.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- '!**/lib/utils/cli.go' | ||
- '!**/lib/utils/testhelpers.go' | ||
- '!**/lib/utils/testutils/**' | ||
- '!**/tool/teleport/testenv/**' |
There was a problem hiding this comment.
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.
There is a lot of shared test code defined outside of _test.go files scattered through out the code base. Dead code detection in Go is not that great and cannot detect that these test helpers are only consumed in tests. This results in testify code being included in production binaries: ```bash strings build/teleport | rg testify github.com/stretchr/testify/assert.init github.com/stretchr/testify/assert.uint32Type go:link.pkghashbytes.github.com/stretchr/testify/assert github.com/stretchr/testify/assert.int64Type github.com/stretchr/testify/assert.uintptrType go:link.pkghashbytes.github.com/stretchr/testify/require github.com/stretchr/testify/assert.stringType github.com/stretchr/testify/assert.float32Type github.com/stretchr/testify/assert.bytesType github.com/stretchr/testify/assert.intType go:link.pkghashbytes.github.com/stretchr/testify/assert/yaml github.com/stretchr/testify/assert.float64Type github.com/stretchr/testify/assert..inittask github.com/stretchr/testify/assert.int32Type github.com/stretchr/testify/assert.uint16Type github.com/stretchr/testify/assert.int8Type github.com/stretchr/testify/assert.uint64Type github.com/stretchr/testify/assert.uintType github.com/stretchr/testify/assert.uint8Type go:link.pkghash.github.com/stretchr/testify/assert/yaml github.com/stretchr/testify/assert.int16Type go:link.pkghash.github.com/stretchr/testify/require go:link.pkghash.github.com/stretchr/testify/assert github.com/stretchr/testify/assert.timeType dep github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= github.com/stretchr/testify/assert.init github.com/stretchr/testify@v1.10.0/assert/assertion_compare.go dep github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= ``` While this doesn't remediate anything, it should hopeful avoid any future code from making it harder to separate test and production code.
4e211ec
to
6cb1db1
Compare
There is a lot of shared test code defined outside of _test.go files scattered through out the code base. Dead code detection in Go is not that great and cannot detect that these test helpers are only consumed in tests. While this doesn't remediate anything, it should hopeful avoid any future code from making it harder to separate test and production code.
Updates #51023.