thanos icon indicating copy to clipboard operation
thanos copied to clipboard

E2E Tests: Consider enabling race detector for test binary

Open matej-g opened this issue 3 years ago • 7 comments

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).

matej-g avatar Sep 16 '21 07:09 matej-g

Yes 👍🏽

bwplotka avatar Sep 20 '21 07:09 bwplotka

Hey, I wanted to work on this issue, can anyone give me some guidance regarding this, thank you!

tend2infinity avatar Jan 06 '22 16:01 tend2infinity

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!

matej-g avatar Jan 07 '22 10:01 matej-g

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

tend2infinity avatar Jan 07 '22 11:01 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.

stale[bot] avatar Apr 17 '22 04:04 stale[bot]

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

matej-g avatar Apr 19 '22 10:04 matej-g

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.

stale[bot] avatar Sep 21 '22 02:09 stale[bot]