-
Notifications
You must be signed in to change notification settings - Fork 16
Kitchensink test #180
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
Kitchensink test #180
Conversation
cd53095 to
1215eaa
Compare
<!--- Note to EXTERNAL Contributors --> <!-- Thanks for opening a PR! If it is a significant code change, please **make sure there is an open issue** for this. We work best with you when we have accepted the idea first before you code. --> <!--- For ALL Contributors 👇 --> ## What was changed <!-- Describe what has changed in this PR --> Move code to build and start worker from `main` into `workers` package. No behavior change expected. ## Why? <!-- Tell your future self why have you made these changes --> Reusability. The [kitchensink parity test](#180) needs to start workers for testing. ## Checklist <!--- add/delete as needed ---> 1. Closes <!-- add issue number here --> 2. How was this tested: <!--- Please describe how you tested your changes/how we can test them --> 3. Any docs updates needed? <!--- update README if applicable or point out where to update docs.temporal.io -->
d83fd6a to
838e3c1
Compare
| scenarioName := r.ScenarioID.Scenario() | ||
| scenario := loadgen.GetScenario(scenarioName) | ||
| if scenario == nil { | ||
| return fmt.Errorf("scenario %v not found", scenarioName) | ||
| } |
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.
Not needed; already done by caller.
|
|
||
| echo "Installing .NET $DOTNET_VERSION..." | ||
| mise use dotnet-core@"$DOTNET_VERSION" | ||
| mise use dotnet@"$DOTNET_VERSION" |
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.
core was not sufficient, actually.
| func (r *workerRunner) preRun() { | ||
| r.builder.preRun() | ||
| r.Runner.Builder = r.builder.Builder | ||
| r.TaskQueueName = loadgen.TaskQueueForRun(r.ScenarioID.Scenario, r.ScenarioID.RunID) |
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.
Moved here to avoid circular dependency on loadgen.
| Scenario string | ||
| RunID string |
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.
Simpler than the getters, actually.
| args = append(args, "--build-arg", arg) | ||
| } | ||
| args = append(args, rootDir()) | ||
| args = append(args, repoDir()) |
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 think repo is clearer than root.
| throw new ApplicationFailure('ExecuteNexusOperation is not supported'); | ||
| } else { | ||
| throw new ApplicationFailure('Unknown action ' + JSON.stringify(action)); | ||
| throw new ApplicationFailure('unrecognized action ' + JSON.stringify(action)); |
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.
The other SDKs all call it that.
838e3c1 to
b8e75e3
Compare
| func BaseDir(repoDir string, lang cmdoptions.Language) string { | ||
| return filepath.Join(repoDir, "workers", lang.String()) | ||
| } |
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.
Made reusable.
PS: Can't hide this detail since the caller often wants to cleanup this dir; if we hide it internally, they don't know what to cleanup.
5eac796 to
a9c0069
Compare
a9c0069 to
aa6de40
Compare
c76810a to
f635e08
Compare
2a22bc0 to
6d3cb6c
Compare
| severity: error | ||
|
|
||
| build-lint-test-go: | ||
| test-omes: |
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.
Added this since the CI wasn't actually running the Omes tests themselves.
6699ae9 to
a7be1b2
Compare
a7be1b2 to
f8a4d45
Compare
Sushisource
left a comment
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.
Dude this is slick. I saw the history matching thing at first and alarm bells went off but when I saw what it was, it's cool. A lot like some of the tests in Core.
| if: ${{ !inputs.do-push || github.event.pull_request.head.repo.fork }} | ||
| with: | ||
| go-version: '^1.24' | ||
| go-version-file: 'go.mod' |
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.
Whaaaat you can do that?
| Fields map[string]any | ||
| } | ||
|
|
||
| func requireHistoryMatches(t *testing.T, actualEvents []*historypb.HistoryEvent, expectedSpec string) { |
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.
Doc of the expected spec would be very welcome here. I was worried at first this was a much tighter match than it actually is.
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.
Good call! 👍 The whole thing is inspired by Alex's version in OSS Server, but much much shorter (partly thanks to Claude).
b177b99 to
eeb920e
Compare
| WorkflowExecutionStarted | ||
| WorkflowTaskScheduled | ||
| WorkflowTaskStarted | ||
| WorkflowTaskCompleted | ||
| TimerStarted {"startToFireTimeout":"0.001s"} | ||
| TimerFired | ||
| WorkflowTaskScheduled | ||
| WorkflowTaskStarted | ||
| WorkflowTaskCompleted | ||
| WorkflowExecutionCompleted`, |
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.
Server has printHistory helper func to print history in this format. You might want it here too.
What was changed
Framework for checking kitchensink actions across SDKs.
(more tests to be added later ...)
Why?
To ensure actions are supported correctly and comprehensively; and document missing implementations.
Especially for new actions this helps ensure you've implemented them correctly.
Checklist
Closes
How was this tested: