-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add a few tests on workflow hooks #8800
Conversation
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.
PR Summary
Added test coverage for three workflow hooks, focusing on basic functionality verification of schema computation, workflow version creation, and step creation processes.
- Test for
useComputeStepOutputSchema
in/packages/twenty-front/src/modules/workflow/hooks/__tests__/useComputeStepOutputSchema.test.ts
needs error handling scenarios - Test for
useCreateNewWorkflowVersion
in/packages/twenty-front/src/modules/workflow/hooks/__tests__/useCreateNewWorkflowVersion.test.ts
should verify data structure integrity - Test for
useCreateStep
in/packages/twenty-front/src/modules/workflow/hooks/__tests__/useCreateStep.test.ts
should validate mock function arguments and test non-draft version scenarios - Missing validation for edge cases where input parameters are invalid or undefined
- Tests should verify the integration between
computeStepOutputSchema
and step creation workflow
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
|
||
describe('useComputeStepOutputSchema', () => { | ||
it('should compute schema successfully', async () => { | ||
const mockInput = { stepId: '123' }; |
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.
logic: mockInput doesn't match implementation - useComputeStepOutputSchema expects a full step object, not just stepId
|
||
const { result } = renderHook(() => useCreateNewWorkflowVersion()); | ||
await result.current.createNewWorkflowVersion( | ||
workflowVersionData as unknown as WorkflowVersion, |
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.
style: Double type assertion is unnecessary and could mask type errors. Consider defining proper types or using a single assertion.
expect(mockUpdateOneWorkflowVersion).toHaveBeenCalled(); | ||
expect(mockOpenRightDrawer).toHaveBeenCalled(); |
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.
style: Add expectations for the actual arguments passed to these mocked functions to ensure they're called with correct parameters
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.
Awesome! I love these tests!
We can merge.
As title