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

Make test target failing with race conditions #339

Closed
elsesiy opened this issue Nov 21, 2019 · 7 comments · Fixed by #340
Closed

Make test target failing with race conditions #339

elsesiy opened this issue Nov 21, 2019 · 7 comments · Fixed by #340
Milestone

Comments

@elsesiy
Copy link
Contributor

elsesiy commented Nov 21, 2019

As mentioned in #306 running make test fails with some data races.as
I attached the log output and here's what I did:

  1. Clone repo
  2. make precommit
  3. make test

I also had to modify the shebang as proposed in #336.

Env info:
go version go1.13.4 darwin/amd64

make_test_output.txt

@iredelmeier
Copy link
Member

@elsesiy do you want to pick this up as part of your work for #306 or do you want someone else to? At a glance, it looks like the problem is that the testExporter (and maybe some other metrics test types) is missing a lock.

@jmacd
Copy link
Contributor

jmacd commented Nov 22, 2019

I'll take this one.

@jmacd
Copy link
Contributor

jmacd commented Nov 22, 2019

I'm confused because when I run go test -race directly on this test, it fails for me. But when I run make precommit it does not fail for me locally w/ the same code. Is this flakiness?

@jmacd
Copy link
Contributor

jmacd commented Nov 22, 2019

It looks like the test should wrap a mutex around its records field. I hadn't written the mutex because in the real SDK export is called in a single threaded context, but it's not in this test.

@jmacd
Copy link
Contributor

jmacd commented Nov 22, 2019

Also, I've determined why this hasn't failed very often, since it fails reliably for me on my machine. (1) I wasn't running the make test rule, I thought make precommit would do that for me. (2) the CircleCI machine has very limited concurrency, as opposed to my machine.

@elsesiy
Copy link
Contributor Author

elsesiy commented Nov 22, 2019

@jmacd I noticed that too but after digging through the targets I found that race detection is only done on make test. Thanks for the update!

@jmacd
Copy link
Contributor

jmacd commented Nov 22, 2019

We added race detection to the CI tests. Thanks for finding this. :)

@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants