Add windows integration test #1912
Conversation
wildum
force-pushed
the
integration-tests-windows
branch
from
3c93929 to
44d2d92
Compare
wildum
force-pushed
the
integration-tests-windows
branch
from
44d2d92 to
8fed84c
Compare
| push: | ||
| branches: | ||
| - main | ||
| pull_request: |
There was a problem hiding this comment.
Doesn't this enable workflow for pull requests? is this temporary?
There was a problem hiding this comment.
yep, I did not change it since our discussion, I will remove it before merging to make sure that the test is running properly
| - name: Setup Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: "1.22" |
There was a problem hiding this comment.
Would that become another place where we need to update Go version? What's the process currently to update these?
There was a problem hiding this comment.
yes, and afaik it's just "search and replace" through the codebase :/
| testFolder := "./tests/" | ||
| alloyBinaryPath := "../../../../../build/alloy" | ||
| alloyBinary := "build/alloy" |
There was a problem hiding this comment.
Let's switch to using https://pkg.go.dev/path#Join
| setupEnvironment() | ||
| defer cleanUpEnvironment() |
There was a problem hiding this comment.
Why no setup or clean up on windows?
There was a problem hiding this comment.
can't use the linux images in the windows runner so we are quite limited in terms of environment. For now not using docker and only running one Alloy to run the windows exporter seems to be sufficient. I can rename the function to "setupDockerEnv" and "cleanupDockerEnv" to make it more clear
There was a problem hiding this comment.
I wonder if for clarity we should have two types of tests:
- one running docker compose with dependency projects - maybe "docker-integration-test"
- another one with just Alloy - maybe "standalone-integration-test"
And have them somewhat more clearly separated in the code. Otherwise it becomes a bit convoluted 🤔
There was a problem hiding this comment.
Not sure that it would be really worth it because besides the docker part which is quite small, they would re-use everything. Also I don't think that we need a linux standalone. And maybe one day the windows runner will be able to use WSL 2 and it will work with Mimir linux image or at some point we may want to run a docker windows image and the difference will be gone.
I agree that now it looks a bit convoluted though. I can structure the test better so that it looks less chaotic
| if !filepath.IsAbs(specificTest) && !strings.HasPrefix(specificTest, "./tests/") { | ||
| specificTest = "./tests/" + specificTest | ||
| if !filepath.IsAbs(specificTest) && !strings.HasPrefix(specificTest, testFolder) { | ||
| specificTest = testFolder + specificTest |
There was a problem hiding this comment.
| // sleep for a few seconds before deleting the files to make sure that they are not use anymore | ||
| time.Sleep(5 * time.Second) | ||
|
|
There was a problem hiding this comment.
Instead of sleeps, can we have a retry and backoff (without panic on first failure) to remove all files?
Sleeps are notoriously fragile and should be avoided. A slow machine can sometimes hang for 5 seconds.
PR Description
Add windows integration tests github action and an integration tests for the default collectors of the windows exporter.
The Setup-go step is super slow... (actions/setup-go#495)
Which issue(s) this PR fixes
Fixes #1896