Write more tests #8799
Write more tests #8799
Conversation
Devessier
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR significantly enhances test coverage across workflow-related utilities and removes unused utility functions, focusing on type safety and comprehensive test scenarios.
- Added test coverage for workflow utilities in
/packages/twenty-front/src/modules/workflow/utils/__tests__/including trigger definitions, step names, and record actions - Critical bug in
/packages/twenty-front/src/utils/__tests__/castToString.test.tswhere null test actually checks undefined - Removed
/packages/twenty-front/src/utils/isAnObject.tsand/promise-to-observable.tsutilities which may require codebase updates - Improved type safety in
/packages/twenty-front/src/modules/workflow/search-variables/utils/getTriggerStepName.tsby replacing null checks with isDefined() - Duplicate test descriptions found in
/packages/twenty-front/src/modules/workflow/utils/__tests__/getManualTriggerDefaultSettings.test.ts
💡 (4/5) You can add custom instructions or style guidelines for the bot here!
11 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile
| it('throws when an unknown trigger type is provided', () => { | ||
| expect(() => { | ||
| getTriggerStepName({ | ||
| type: 'unknown' as any, | ||
| name: '', | ||
| settings: { | ||
| objectType: 'company', | ||
| outputSchema: {}, | ||
| }, | ||
| }); | ||
| }).toThrow(); |
There was a problem hiding this comment.
style: test should verify the specific error message thrown, not just that an error occurs
| import { capitalize } from '~/utils/string/capitalize'; | ||
|
|
||
| export const getTriggerStepName = (trigger: WorkflowTrigger): string => { | ||
| switch (trigger.type) { | ||
| case 'DATABASE_EVENT': | ||
| return getDatabaseEventTriggerStepName(trigger); | ||
| case 'MANUAL': | ||
| if (!trigger.settings.objectType) { | ||
| if (!isDefined(trigger.settings.objectType)) { | ||
| return 'Manual trigger'; | ||
| } | ||
|
|
||
| return 'Manual trigger for ' + capitalize(trigger.settings.objectType); |
There was a problem hiding this comment.
style: String concatenation replaced with template literal for consistency with line 29
|
|
||
| it('throws when argument is not never', () => { | ||
| expect(() => { | ||
| assertUnreachable(42 as never); |
There was a problem hiding this comment.
style: casting 42 to never type is not a realistic test case - consider using a more representative example like an unhandled enum case
| it('throws when provided workflow is undefined', () => { | ||
| expect(() => { | ||
| assertWorkflowWithCurrentVersionIsDefined(undefined); | ||
| }).toThrow(); |
There was a problem hiding this comment.
style: Consider checking for a specific error message to ensure the right error is thrown
| it('returns settings for a manual trigger that can be activated from any where', () => { | ||
| expect( | ||
| getManualTriggerDefaultSettings({ | ||
| availability: 'EVERYWHERE', | ||
| activeObjectMetadataItems: generatedMockObjectMetadataItems, | ||
| }), | ||
| ).toStrictEqual({ | ||
| objectType: undefined, | ||
| outputSchema: {}, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
style: Test description is duplicated with the test below. Should be 'returns settings for EVERYWHERE availability' instead.
| import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems'; | ||
| import { getStepDefaultDefinition } from '../getStepDefaultDefinition'; |
There was a problem hiding this comment.
logic: Missing test for when activeObjectMetadataItems is empty or undefined
| type: 'DATABASE_EVENT', | ||
| activeObjectMetadataItems: [], | ||
| }); | ||
| }).toThrow(); |
There was a problem hiding this comment.
logic: Test should verify the exact error message thrown matches implementation: 'This function need to receive at least one object metadata item to run.'
| }); | ||
|
|
||
| it('returns false for Record Update', () => { | ||
| const codeAction: WorkflowRecordCRUDAction = { |
There was a problem hiding this comment.
style: variable name 'codeAction' is misleading since this is a CRUD action
| }); | ||
|
|
||
| it('returns an empty string when null is provided', () => { | ||
| expect(castToString(undefined)).toBe(''); |
There was a problem hiding this comment.
logic: This test claims to test null but is actually testing undefined. Should be castToString(null) instead of castToString(undefined)
There was a problem hiding this comment.
logic: Removing this function could break type checking in other parts of the codebase. Need to verify all usages are updated to handle empty objects correctly. Consider using TypeScript's type system or Object.prototype.toString.call() instead.
|
Thanks @Devessier for your contribution! |
No description provided.