test: (temporal) add unit tests for temporal workflow activities #278
Conversation
WalkthroughThis pull request introduces three new test files in the Changes
Sequence DiagramsequenceDiagram
participant Test as Test Suite
participant Activities as Activities Struct
participant TemporalClient as Temporal Client
Test->>Activities: Initialize with Mocks
Test->>Activities: Call Method (e.g., TemporalScheduleCreate)
Activities->>TemporalClient: Invoke Client Method
TemporalClient-->>Activities: Return Result/Error
Activities-->>Test: Propagate Result/Error
Test->>Test: Assert Expected Behavior
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
79aad5c to
d142247
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
==========================================
+ Coverage 72.91% 72.96% +0.04%
==========================================
Files 537 537
Lines 26988 26988
==========================================
+ Hits 19679 19691 +12
+ Misses 6248 6235 -13
- Partials 1061 1062 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
internal/connectors/engine/activities/temporal_workflow_terminate_test.go (2)
7-16: Unused imports
The import paths here look consistent with the test's usage, except it appears there's no direct usage ofeventsin this file. Ifevtsisn't needed, you might consider removing both the variable and the import statements to avoid clutter.- "github.com/formancehq/payments/internal/events" ... - evts *events.Events
18-31: Suggestion: Wrap in a 'Context' block for clarity
While this works fine, consider grouping the related variables (workflowID,runID,reason) in aContextblock or a dedicatedDescribeto enhance readability when scaling up test scenarios in the future.internal/connectors/engine/activities/temporal_workflow_executions_list_test.go (4)
31-45: Consider initializing or removingevtsif unused.
Havingevtsdeclared without assignment may lead to confusion. If it's not needed for these tests, please remove it. Otherwise, initialize it properly to ensure clarity.-evts *events.Events ... -act = activities.New(logger, t, s, evts, p, time.Millisecond) +// If 'evts' is not used in tests, remove its declaration and pass nil explicitly +// or if it is required, initialize it. For example: +evts := events.New(...) +act = activities.New(logger, t, s, evts, p, time.Millisecond)
12-13: Unify or clarify the use of two mocking frameworks.
The test imports both"github.com/golang/mock/gomock"(aliased aslegacy_gomock) and"go.uber.org/mock/gomock"(aliased asgomock). Maintaining two versions of gomock might cause confusion. Consider unifying them if there is no specific need for both.Also applies to: 17-18
52-57: Validate error content for deeper test coverage.
While verifying that the error is not nil is sufficient for presence checks, confirming that the error message matches an expected substring or error type can provide more robust test coverage and clarity.-Expect(err).NotTo(BeNil()) +Expect(err).To(HaveOccurred()) +Expect(err.Error()).To(ContainSubstring("some error"))
68-72: Double-check the request argument with stricter matching.
Currently,ListWorkflowExecutionsis invoked withgomock.Any(). Using argument matchers likegomock.Eq(req)or a more specific custom matcher can ensure the request object is passed correctly.-w.EXPECT().ListWorkflowExecutions(ctx, gomock.Any()).Return(expectedRes, nil) +w.EXPECT().ListWorkflowExecutions(ctx, gomock.Eq(req)).Return(expectedRes, nil)internal/connectors/engine/activities/temporal_schedule_create_test.go (4)
19-24: Add coverage for additional schedule options
Currently, this matcher covers only a subset of the schedule options. If theTemporalScheduleCreateactivity later includes other fields (e.g.,CalendarSpecs,CronExpressions, or any advanced scheduling features), you might miss regressions. Consider expanding the test matcher coverage to ensure that all critical fields are checked.
47-49: Include all fields inString()method for enhanced diagnostic output
The currentString()method only displays the schedule ID and trigger flag, which can make it harder to debug failures if other fields are mismatched. Including the overlap policy, jitter, and additional fields in the output could reduce debugging time.
76-88: Check for specific error messages to confirm error-handling logic
While checking thaterris non-nil confirms a failure, you might also verify that the error message or type is what you expect (e.g., confirming that it's a Temporal client error vs. a different error). This can increase confidence in correct error-handling paths.
89-107: Consider adding test coverage for different schedule creation edge cases
For improved test coverage, include variations such asTriggerImmediately = false, an invalid overlap policy, or an intentionally emptyScheduleID. This helps ensureTemporalScheduleCreatebehaves correctly under all valid and invalid conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (3)
internal/connectors/engine/activities/temporal_schedule_create_test.go(1 hunks)internal/connectors/engine/activities/temporal_workflow_executions_list_test.go(1 hunks)internal/connectors/engine/activities/temporal_workflow_terminate_test.go(1 hunks)
🔇 Additional comments (5)
internal/connectors/engine/activities/temporal_workflow_terminate_test.go (3)
47-53: Good test coverage for unhandled errors
Verifying thatTemporalWorkflowTerminatecorrectly propagates unhandled errors ensures robust error handling.
54-59: Workflow not found scenario
It's a good pattern to ignore not-found errors, preventing the test from failing if the workflow is already terminated or missing.
61-66: Successful workflow termination
This test scenario is concise and confirms correct behavior when no error is returned.internal/connectors/engine/activities/temporal_schedule_create_test.go (2)
41-45: Guard against potential nil pointer access inopts.Spec
Though rare, ifopts.Specis ever nil,opts.Spec.Jittercould panic. To be safer, consider a short-circuit check or add an additional condition to prevent nil pointer dereferencing.
65-74: Verify the unusedevtsdependency initialization
You declareevtsbut never assign it before passing it toactivities.New(...). If this is intentional, consider removingevtsfrom the constructor for clarity, or fully initialize it if needed for upcoming tests.
No description provided.