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

remove redundant tests #1677

Merged

Conversation

xavpaice
Copy link
Member

@xavpaice xavpaice commented Nov 7, 2024

Description, Motivation and Context

Removes some test redundancy, and golangci-lint

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

@xavpaice xavpaice requested a review from a team as a code owner November 7, 2024 04:58
DexterYan
DexterYan previously approved these changes Nov 7, 2024
@xavpaice
Copy link
Member Author

xavpaice commented Nov 7, 2024

The 3 tests waiting to be reported are the ones I've removed in this PR.

@xavpaice xavpaice linked an issue Nov 7, 2024 that may be closed by this pull request
@@ -237,18 +237,6 @@ scan:
--ignore-unfixed \
./

.PHONY: lint
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the aim not to run golangci-lint in CI? I think we want to keep this, for manually running it.

Copy link
Member Author

Choose a reason for hiding this comment

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

the message we got was that "Org wide, we are not recommending or using golangci-lint". As per this, I've removed it entirely. The results aren't that useful to us.

@@ -27,25 +27,6 @@ jobs:
- name: Fails in order to indicate that pull request needs to be marked as ready to review and unit tests workflow needs to pass.
run: exit 1

test:
Copy link
Member

Choose a reason for hiding this comment

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

I think we ought to leave at least make vet. we could add it to an existing action such as test-integration

Copy link
Member Author

Choose a reason for hiding this comment

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

go test runs vet as step 1 regardless, iiuc.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://pkg.go.dev/cmd/go/internal/test "As part of building a test binary, go test runs go vet on the package
and its test source files" - though it does run a subset.

The Makefile that is called with make test includes the make vet as a dependency, I'll add the same to make test-integration.

@xavpaice xavpaice force-pushed the xavpaice/sc-114967/remove-golangci-lint-from-troubleshoot-ci branch from 4b03f8f to 747c88e Compare November 8, 2024 01:22
Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

LGTM

@banjoh
Copy link
Member

banjoh commented Nov 8, 2024

The 3 tests waiting to be reported are the ones I've removed in this PR.

I needed to update Branch protection rules -> Require status checks to pass before merging to reflect this PR changes

@banjoh banjoh merged commit 2f62240 into main Nov 8, 2024
24 checks passed
@banjoh banjoh deleted the xavpaice/sc-114967/remove-golangci-lint-from-troubleshoot-ci branch November 8, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove golangci-lint from CI
3 participants