thanos
thanos copied to clipboard
E2E Tests: Consider enabling race detector for test binary
Enabling race detector for E2E tests could help us in catching concurrency problems early on (see context here: https://github.com/thanos-io/thanos/issues/4579#issuecomment-917716313).
We could introduce a separate Dockerfile to build image specifically for testing, with separate promu
config (we'll need to enable CGO and use a non-Alpine based image).
Yes 👍🏽
Hey, I wanted to work on this issue, can anyone give me some guidance regarding this, thank you!
Hey @tend2infinity, I'd say go for it!
As for guidance, it is written in the description in a kind of condensed form, but I believe we need to:
- Adjust (or probably create a new one really) Dockerfile which will take as a base image something else other than the Alpine image - the problem is the race detector depends on having CGO enabled, which is not available in the Alpine image.
- Add a new configuration for
promu
(utility which Thanos uses to build binaries) to ensure the binary for testing is actually build with CGO enabled. Have a look at current config in.promu.yml
to get the picture. - Adjust the
make
targets to use the new Dockerfile / build configuration for E2E tests.
If I still haven't cleared out something, feel free to ask!
Hey thanks a lot @matej-g , I was actually kinda confused but now it seems clear, I'll ask if I get stuck into something
Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind
command if you wish to be reminded at some point in future.
This is actually in-progress, we just need a bit more :heart: to resolve races that have already been detected, see my comment in https://github.com/thanos-io/thanos/pull/5066#issuecomment-1056935086. These should be addressed before the race detection is merged, otherwise we'll (logically) end up with failing CI tests. Hopefully I'll (or someone else?) will be able to find the time to take a look soon :disappointed:.
cc @tend2infinity
Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind
command if you wish to be reminded at some point in future.