Skip to content

Conversation

@zsiec
Copy link
Member

@zsiec zsiec commented May 11, 2019

I'm new to this repository and testing out the features currently implemented. After a clone, I noticed that while go test ./... was passing, when adding the -race param, you encounter the following race:

==================
WARNING: DATA RACE
Write at 0x00c00001f620 by goroutine 33:
  github.com/NYTimes/video-transcoding-api/db/redis.(*redisRepository).CreateJob()
      /Users/Symborski/dev/video-transcoding-api/db/redis/job.go:19 +0x1ad
  github.com/NYTimes/video-transcoding-api/db/redis.TestCreateJobIsSafe.func1()
      /Users/Symborski/dev/video-transcoding-api/db/redis/job_test.go:101 +0x96

Previous write at 0x00c00001f620 by goroutine 25:
  github.com/NYTimes/video-transcoding-api/db/redis.(*redisRepository).CreateJob()
      /Users/Symborski/dev/video-transcoding-api/db/redis/job.go:19 +0x1ad
  github.com/NYTimes/video-transcoding-api/db/redis.TestCreateJobIsSafe.func1()
      /Users/Symborski/dev/video-transcoding-api/db/redis/job_test.go:101 +0x96

Goroutine 33 (running) created at:
  github.com/NYTimes/video-transcoding-api/db/redis.TestCreateJobIsSafe()
      /Users/Symborski/dev/video-transcoding-api/db/redis/job_test.go:99 +0x3b9
  testing.tRunner()
      /usr/local/Cellar/go/1.11.5/libexec/src/testing/testing.go:827 +0x162

Goroutine 25 (running) created at:
  github.com/NYTimes/video-transcoding-api/db/redis.TestCreateJobIsSafe()
      /Users/Symborski/dev/video-transcoding-api/db/redis/job_test.go:99 +0x3b9
  testing.tRunner()
      /usr/local/Cellar/go/1.11.5/libexec/src/testing/testing.go:827 +0x162
==================

I made a quick change to address this issue.

~/dev/video-transcoding-api ➤ c79d88c|bug-test-datarace⚡ ± go test ./... -race
?   	github.com/NYTimes/video-transcoding-api	[no test files]
ok  	github.com/NYTimes/video-transcoding-api/config	(cached)
ok  	github.com/NYTimes/video-transcoding-api/db	(cached)
ok  	github.com/NYTimes/video-transcoding-api/db/dbtest	(cached)
ok  	github.com/NYTimes/video-transcoding-api/db/redis	(cached)
ok  	github.com/NYTimes/video-transcoding-api/db/redis/storage	1.073s
ok  	github.com/NYTimes/video-transcoding-api/provider	(cached)
ok  	github.com/NYTimes/video-transcoding-api/provider/bitmovin	(cached)
ok  	github.com/NYTimes/video-transcoding-api/provider/elastictranscoder	(cached)
ok  	github.com/NYTimes/video-transcoding-api/provider/elementalconductor	(cached)
ok  	github.com/NYTimes/video-transcoding-api/provider/encodingcom	(cached)
?   	github.com/NYTimes/video-transcoding-api/provider/hybrik	[no test files]
ok  	github.com/NYTimes/video-transcoding-api/provider/zencoder	(cached)
ok  	github.com/NYTimes/video-transcoding-api/service	(cached)
ok  	github.com/NYTimes/video-transcoding-api/swagger	(cached)

While I'm not 100% sure what the true purpose TestCreateJobIsSafe() is, the race may constitute a failure, but I'd look to the author or maintainers for guidance.

@codecov-io
Copy link

codecov-io commented May 11, 2019

Codecov Report

Merging #208 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #208   +/-   ##
=======================================
  Coverage   78.62%   78.62%           
=======================================
  Files          29       29           
  Lines        2914     2914           
=======================================
  Hits         2291     2291           
  Misses        372      372           
  Partials      251      251

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1fda45...2585bec. Read the comment docs.

@zsiec
Copy link
Member Author

zsiec commented May 11, 2019

There's a test failure with golang at master that looks to be a preexisting condition. Will see if I can track that down too in another PR.

@fsouza fsouza merged commit b5476bd into video-dev:master May 12, 2019
@fsouza
Copy link
Collaborator

fsouza commented May 12, 2019

@zsiec thanks for fixing this! I'll add tip to allow_failures and investigate what's going on with the swagger validation on Monday or Tuesday x)

@zsiec zsiec deleted the bug-test-datarace branch May 12, 2019 06:14
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 this pull request may close these issues.

3 participants