-
Notifications
You must be signed in to change notification settings - Fork 900
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
Run integration tests again #7304
Conversation
9380837
to
c175dc9
Compare
@@ -1187,7 +1187,7 @@ func TestSQLiteTransactionContextCancellation(t *testing.T) { | |||
cancel() | |||
|
|||
err = tx.Commit() | |||
assert.ErrorAs(t, err, &context.Canceled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This yields a data race when the assertion library is trying to wrap the error.
Example: https://github.com/temporalio/temporal/actions/runs/13249022301/job/36982452711?pr=7262#step:7:27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this could possible work at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷
c175dc9
to
75f221e
Compare
@@ -309,9 +297,8 @@ func TestDLQCommand_V2(t *testing.T) { | |||
}, | |||
} { | |||
t.Run(tc.name, func(t *testing.T) { | |||
t.Parallel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No data race if you don't run it in parallel :smart:
Makefile
Outdated
@@ -413,13 +413,13 @@ prepare-coverage-test: $(GOTESTSUM) $(TEST_OUTPUT_ROOT) | |||
|
|||
unit-test-coverage: prepare-coverage-test | |||
@printf $(COLOR) "Run unit tests with coverage..." | |||
go run ./cmd/tools/test-runner $(GOTESTSUM) -retries $(FAILED_TEST_RETRIES) --junitfile $(NEW_REPORT) --packages $(UNIT_TEST_DIRS) -- \ | |||
go run ./cmd/tools/test-runner $(GOTESTSUM) -retries $(FAILED_TEST_RETRIES) --junitfile $(NEW_REPORT) --packages="$(UNIT_TEST_DIRS)" -- \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you need =
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but it aligns with the change I'm making in the code coverage PR (where I'm changing all of them to use =
for easier parsing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add them for other flags on this line then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't yet, because testrunner expects them to be like this right now.
It actually supports both 👍
a47d46b
to
d04305d
Compare
d04305d
to
cfe4635
Compare
What changed?
Run persistence integration test again.
Why?
They should be running 🙃 Somehow they were omitted when either
gotestsum
orgo test
parse the space-separated list of packages to test.How did you test it?
Potential risks
Documentation
Is hotfix candidate?