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

all: enable -race flag for tests #336

Merged
merged 15 commits into from
Aug 11, 2021
Merged

all: enable -race flag for tests #336

merged 15 commits into from
Aug 11, 2021

Conversation

igungor
Copy link
Member

@igungor igungor commented Aug 4, 2021

Below change will slowdown the tests quite a lot. Let's see the numbers
first, then we can think of a solution if necessary.

--- a/e2e/util_test.go
+++ b/e2e/util_test.go
@@ -186,6 +186,7 @@ func goBuildS5cmd() func() {
 	cmd := exec.Command(
 		"go", "build",
 		"-mod=vendor",
+		"-race",
 		"-o", s5cmdPath,
 	)

Fix #329

igungor added 6 commits August 3, 2021 16:46
The `run` command iterates over the commands list, where each item is a
pointer to the command. `urfave/cli` packages implicitly adds `-h` flag
to each command, which introduces data race because `s5cmd` executes
them in parallel.

Fixes #301
Below change will slowdown the tests quite a lot. Let's see the numbers
first, then we can think of a solution if necessary.

Fix #329

```patch
--- a/e2e/util_test.go
+++ b/e2e/util_test.go
@@ -186,6 +186,7 @@ func goBuildS5cmd() func() {
 	cmd := exec.Command(
 		"go", "build",
 		"-mod=vendor",
+		"-race",
 		"-o", s5cmdPath,
 	)
```
@igungor
Copy link
Member Author

igungor commented Aug 4, 2021

Tests failed on Windows.

==2688==ERROR: ThreadSanitizer failed to allocate 0x000001000000 (16777216) bytes at 0x040140000000 (error code: 1455)

Found an open ticket: golang/go#22553

@igungor
Copy link
Member Author

igungor commented Aug 5, 2021

Tests finish around 5 minutes on each race-enabled platforms. Overall CI checks finished about in 32 minutes. It's bad, though I've found the bottleneck.

TestName                                          Duration_In_Seconds
TestCatS3Object/cat_remote_object_with_json_flag  197
TestCatS3Object/cat_remote_object                 197
TestCopyLocalFileToS3WithFilePermissions          11
TestCatS3Object                                   11
TestMoveMultipleS3ObjectsToS3                     5

https://gist.github.com/igungor/49f42e6cf457c309a85a26c9e18c56f6

@igungor igungor marked this pull request as ready for review August 5, 2021 17:13
@igungor igungor requested a review from a team as a code owner August 5, 2021 17:13
@igungor igungor requested review from ilkinulas and sonmezonur and removed request for a team August 5, 2021 17:13
@igungor
Copy link
Member Author

igungor commented Aug 11, 2021

@ilkinulas @sonmezonur PTAL.

@igungor igungor merged commit fb03bce into master Aug 11, 2021
@igungor igungor deleted the enable-race-flag branch August 11, 2021 13:08
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.

Enable -race flag for make test target
3 participants