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

[ci] Lint on non-test code importing packages from /tests #3214

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

marun
Copy link
Contributor

@marun marun commented Jul 22, 2024

This avoids having to redundantly tag test code in /tests with go:build test or supply --tags test on every ginkgo invocation.

How was this tested

  • Negative case validated in CI
  • Positive case manually validated by adding a reference to a package under /tests to non-test code and witnessing a failure:
Non-test Go files importing test-only packages MUST have '//go:build test' tag:
/home/me/src/avalanchego/master/main/main.go

@marun marun added the ci This focuses on changes to the CI process label Jul 22, 2024
@marun marun self-assigned this Jul 22, 2024
Copy link
Contributor

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

  • Approving with one change requested

This is much cleaner. Thanks for the addition!

scripts/lint.sh Outdated Show resolved Hide resolved
@StephenButtolph StephenButtolph added this to the v1.11.11 milestone Jul 26, 2024
@marun
Copy link
Contributor Author

marun commented Jul 26, 2024

Rebased

scripts/lint.sh Outdated
IMPORT_FROM_TESTS=$( echo "${NON_TEST_GO_FILES}" | xargs grep -l '"github.com/ava-labs/tests/');
IMPORT_TEST_PKG=$( echo "${NON_TEST_GO_FILES}" | xargs grep -lP '"github.com/ava-labs/.*?test"');
IMPORT_FROM_TESTS=$( echo "${NON_TEST_GO_FILES}" | xargs grep -l '"github.com/ava-labs/avalanchego/tests/');
IMPORT_TEST_PKG=$( echo "${NON_TEST_GO_FILES}" | xargs grep -lP '"github.com/ava-labs/avalanchego/.*?test');
Copy link
Contributor

Choose a reason for hiding this comment

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

The closing double-quote was deliberate, to ensure that the package name ends in test. This will now just allow anything with test in the path to be ok.

Is that something we're ok with? I tend to err on the side of precision, but can see the benefit in opening it up like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we're in bikeshedding territory either way, so I'll revert the removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

This avoids having to redundantly tag test code in `/tests` with
`go:build test` or supply `--tags test` on every ginkgo invocation.
@StephenButtolph StephenButtolph added this pull request to the merge queue Jul 26, 2024
Merged via the queue into master with commit 5abdebd Jul 26, 2024
20 checks passed
@StephenButtolph StephenButtolph deleted the ci-lint-tests-import branch July 26, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci This focuses on changes to the CI process
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants