Allow fix tests to run concurrently #1576
Conversation
ashanbrown
added
area: tests
| @@ -27,7 +27,9 @@ func TestFix(t *testing.T) { | |||
| if os.Getenv("GL_KEEP_TEMP_FILES") == "1" { | |||
| t.Logf("Temp dir for fix test: %s", tmpDir) | |||
| } else { | |||
| defer os.RemoveAll(tmpDir) | |||
| t.Cleanup(func() { | |||
There was a problem hiding this comment.
t.Cleanup waits for all tests to run, whereas defer does not.
ashanbrown
force-pushed
the
asb/parallel-fix-test
branch
4 times, most recently
from
872c5ac to
e2fe6a3
Compare
test/register_cleanup.go
Outdated
| @@ -0,0 +1,12 @@ | |||
| // +build go1.14 | |||
There was a problem hiding this comment.
I'm not completely sure whether it's worth using build tags just to make this work but it is annoying how long the fix tests to run when doing development, so I think it's worth considering.
|
I think that supporting the build of the project with an unmaintained version of Go is useless, encourages bad practice, and creates unnecessary complexity. IMHO we can drop the support of go1.13, go1.16 will come soon so I think it's the right moment to drop go1.13. |
|
We can drop 1.13 support if it will really help us to simplify code. I'd better wait until go1.16 if "hack" for 1.13 can be isolated and not overly complicates implementation. |
|
Oops, wrong button, sorry |
|
I don't have a simpler hack than these build tags but I'm open to suggestions. I'm also happy to sit in this until go 1.16 is released in February. |
|
Build tags are ok. |
|
@ldez What's your take on this? Ok to merge for now or would you rather sit on this until go1.16 is released? I'm happy to patch it up once go1.16 is out. I could even put up a PR immediately and we could merge it in a few months. |
|
Let's merge it and cleanup later. I've re-checked and 10-12 lines of version specific code is minor overhead. |
|
@ldez Do you have any concerns about merging this as is or would you rather we wait? As mentioned, I can put up a PR to clean this up immediately, to be merged when we stop supporting go 1.13. |
ashanbrown
changed the title
Ensure cleanup doesn't happen until after all tests are done. Also don't require holding a lock since files are not shared between tests.
ldez
force-pushed
the
asb/parallel-fix-test
branch
from
32a6393 to
9606ad9
Compare
ldez
changed the title
I noticed the "fix" tests were taking a while to run because they don't run concurrently. It would take about 12 seconds before and with this change takes about 4 on my local machine.
This change does two things:
I admittedly don't know what resources the shared file lock is designed to protect, but I suspect (2) is safe. This PR does not set
t.Parallel()at the top-level ofTestFix(). It would be nice to do that but again I don't know how that would play with disabling use of the file lock.