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

E2E Tests: Consider enabling race detector for test binary #4664

Closed
matej-g opened this issue Sep 16, 2021 · 7 comments · Fixed by #6873
Closed

E2E Tests: Consider enabling race detector for test binary #4664

matej-g opened this issue Sep 16, 2021 · 7 comments · Fixed by #6873
Labels
difficulty: easy dont-go-stale Label for important issues which tells the stalebot not to close them help wanted tests

Comments

@matej-g
Copy link
Collaborator

matej-g commented Sep 16, 2021

Enabling race detector for E2E tests could help us in catching concurrency problems early on (see context here: #4579 (comment)).

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

@bwplotka
Copy link
Member

Yes 👍🏽

@tend2infinity
Copy link
Contributor

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

@matej-g
Copy link
Collaborator Author

matej-g commented Jan 7, 2022

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!

@tend2infinity
Copy link
Contributor

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

@stale
Copy link

stale bot commented Apr 17, 2022

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 stale bot added the stale label Apr 17, 2022
@matej-g
Copy link
Collaborator Author

matej-g commented Apr 19, 2022

This is actually in-progress, we just need a bit more ❤️ to resolve races that have already been detected, see my comment in #5066 (comment). 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 😞.

cc @tend2infinity

@stale
Copy link

stale bot commented Sep 21, 2022

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 stale bot added the stale label Sep 21, 2022
@bwplotka bwplotka added the dont-go-stale Label for important issues which tells the stalebot not to close them label Oct 19, 2022
@stale stale bot removed the stale label Oct 19, 2022
GiedriusS added a commit that referenced this issue Nov 4, 2023
Let's enable the race detection for unit tests. Closes
#4664.

Signed-off-by: Giedrius Statkevičius <[email protected]>
yeya24 pushed a commit that referenced this issue Nov 4, 2023
Let's enable the race detection for unit tests. Closes
#4664.

Signed-off-by: Giedrius Statkevičius <[email protected]>
rabenhorst pushed a commit to rabenhorst/thanos that referenced this issue Nov 15, 2023
Let's enable the race detection for unit tests. Closes
thanos-io#4664.

Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Sebastian Rabenhorst <[email protected]>
douglascamata pushed a commit to douglascamata/thanos that referenced this issue Dec 13, 2023
Let's enable the race detection for unit tests. Closes
thanos-io#4664.

Signed-off-by: Giedrius Statkevičius <[email protected]>
douglascamata pushed a commit to douglascamata/thanos that referenced this issue Dec 13, 2023
Let's enable the race detection for unit tests. Closes
thanos-io#4664.

Signed-off-by: Giedrius Statkevičius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy dont-go-stale Label for important issues which tells the stalebot not to close them help wanted tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants