-
Notifications
You must be signed in to change notification settings - Fork 505
chore: Move integration tests and update MakeFile how we run our tests #5289
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
base: main
Are you sure you want to change the base?
Conversation
|
We can now run our tests on windows. The tests that are failing are already noted down here: #5142 Can address them in a followup pr, don't want it to block this one |
ptodev
left a comment
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'm worried that if someone adds a test that's not in /internal, then there's a chance that they won't realise their tests are not being ran by the CI. Isn't it customary to be able to run tests from the same folder where the go.mod is?
Ideally, there should be a good way to exclude integration tests from being ran on windows via the cmd line. If there isn't a good way to do so, we could use comments like //go:build !windows? Those tests use k8s and docker which are more of a Linux thing anyway. To make this more explicit, you could rename the integration-tests folder to integration-tests-linux? In the future we could add another folder called integration-tests-windows to test Windows-specific features.
integration-tests/k8s/tests/mimir-alerts-kubernetes/testdata/expected_1.yml
Show resolved
Hide resolved
| - name: Test | ||
| run: '& "C:/Program Files/git/bin/bash.exe" -c ''go test -tags="nodocker,nonetwork" | ||
| $(go list ./... | grep -v -E "/integration-tests/|/integration-tests-k8s/")''' | ||
| run: '& "C:/Program Files/git/bin/bash.exe" -c ''go test -tags="nodocker,nonetwork" ./internal/...''' |
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.
./internal/... is not enough I think because there are more top-level directories that may now or in the future contain more tests.
We could list all the top-level directories. Or we could use some script to do that automatically and exclude integration-tests/. Or we can use some other go test setting / env variable to skip the integration tests.
|
@ptodev @thampiotr in a4d50ad I updated to set I just need to figure out how we can do this for windows too. I also need to add back to manually trigger node_exporter test because it has !race flag. Let me know what you think |
One issue with tag |
Pull Request Details
We have a issue running our tests on windows
Essentially the output of
$(go list ./... | grep -v -E "/integration-tests/|/integration-tests-k8s/")is to large to pass as arguments for windows.We run this command to exclude our integration tests. To remove this workaround I moved all integration tests from
internal/cmdtotestsfolder. Somake testswill now run all tests ininternal,extentions/alloyengineandcollector. The last two are have their own go mod and we never triggered these tests in ci before.Issue(s) fixed by this Pull Request
Notes to the Reviewer
PR Checklist