IWF-186: Upgrade Temporal SDK #442
Conversation
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-186
branch
from
c555576
to
42c10b7
Compare
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-186
branch
from
42c10b7
to
456682c
Compare
|
To review: |
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-186
branch
2 times, most recently
from
739ee79
to
456682c
Compare
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-186
branch
from
53f052c
to
7d030dc
Compare
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-186
branch
from
bde2eaa
to
f44cff1
Compare
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-186
branch
from
2e5220d
to
0399451
Compare
| @@ -116,7 +116,7 @@ func doTestLockingWorkflow(t *testing.T, backendType service.BackendType, config | |||
| ValueType: iwfidl.INT.Ptr(), | |||
| }, | |||
| } | |||
| time.Sleep(time.Second * 1) | |||
| time.Sleep(time.Second * 2) | |||
There was a problem hiding this comment.
Test passes locally with no change. I had to bump it to make the test pass in Github CI
There was a problem hiding this comment.
I am still not sure why we need more sleep. I remember there is a new behavior in temporal cloud that they now enforce a backoff of 1s for each continueAsNew to prevent having too fast continueAsNew which cause hot partition in server. Maybe that's the cause. we can dig into the release logs to confirm, or maybe ask them
There was a problem hiding this comment.
actually the minimum backoff has been there for a long time: https://github.com/temporalio/temporal/blame/fad6bdc0e9c0911f28829f3c47285357554e2567/service/history/workflow/mutable_state_impl.go#L1703
There was a problem hiding this comment.
I'm looking into this, but it's not easy to troubleshoot since it passes locally
There was a problem hiding this comment.
I just looked into that, looks like there is a test panic at timeout for the cadence cases
| @@ -108,7 +108,7 @@ func doTestAnyCommandCombinationWorkflow(t *testing.T, backendType service.Backe | |||
| panicAtHttpError(err, httpResp) | |||
|
|
|||
| // skip the timer for S1 | |||
| time.Sleep(time.Second * 2) // wait for a second so that timer is ready to be skipped | |||
| time.Sleep(time.Second * 5) // wait for a few seconds so that timer is ready to be skipped | |||
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-186
branch
from
2d48ae6
to
36c8d58
Compare
| # Fails CI when used with -coverprofile flag due to tests that panic; see https://go.dev/doc/build-cover#panicprof | ||
| # $Q go test -v ./... -timeout 15m -cover -coverprofile coverage.out -coverpkg ./service/... | ||
| $Q go test -v ./... -timeout 15m |
There was a problem hiding this comment.
@longquanzheng CI issue is caused by Go upgrade from 1.19 to 1.22
Beginning in Go 1.20, Go supports collection of coverage profiles from applications and from integration tests, larger and more complex tests for Go programs.
If my program panics, will coverage data be written?¶
Programs built with go build -cover will only write out complete profile data at the end of execution if the program invokes os.Exit() or returns normally from main.main. If a program terminates in an unrecovered panic, or if the program hits a fatal exception (such as a segmentation violation, divide by zero, etc), profile data from statements executed during the run will be lost.
When tests panic, the task returns "Error 1" which fails the Github Action. We should fix it by refactoring tests, but I removed the coverage options to unblock for now
There was a problem hiding this comment.
Good finding! Thanks!!!
Description
Checklist
Related Issue
Closes #434