test(workflow): add integration tests for workflow controller #2888
Conversation
- Create integration tests for the external workflow controller - Validate authentication and input parameters in various scenarios (your test suite is so long, I half expect it to come with a subscription plan)
|
WalkthroughThe pull request introduces a suite of integration tests for the external workflow controller in a NestJS application, specifically targeting the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts (3)
1-175: LGTM! Test setup is comprehensive and well-structured.The test setup follows best practices with proper isolation and dependencies initialization. Consider extracting the test data creation into helper functions for better reusability across test files.
Consider refactoring the test data creation into helper functions:
const createTestWorkflowDefinition = async (workflowDefinitionRepository: WorkflowDefinitionRepository, projectId: string) => { return await workflowDefinitionRepository.create({ data: { id: 'workflow-id', name: 'workflow-name', definitionType: 'statechart-json', definition: {}, project: { connect: { id: projectId, }, }, }, }); };
177-199: LGTM! Authentication test cases are well-defined.The tests properly validate unauthorized access scenarios. Consider enhancing the error message validation to ensure proper error responses.
Add error message validation:
it('should return 401 when not recieving authorization token', async () => { const res = await request(app.getHttpServer()) .post('/external/workflows/run') .send({}); expect(res.statusCode).toEqual(401); + expect(res.body).toEqual({ + message: 'Unauthorized', + statusCode: 401 + }); });
201-287: LGTM! Error test cases are comprehensive and well-structured.The tests thoroughly validate various error scenarios with proper error messages. Consider reducing duplication in test setup.
Extract common test setup:
const makeAuthenticatedRequest = (app: INestApplication, data: any) => { return request(app.getHttpServer()) .post('/external/workflows/run') .set('authorization', `Bearer ${API_KEY}`) .send(data); }; it('should return 400 when there is no context', async () => { const res = await makeAuthenticatedRequest(app, {}); expect(res.statusCode).toEqual(400); expect(res.body.message).toEqual('Context is required'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.controller.external.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.ts(1 hunks)
🔇 Additional comments (1)
services/workflows-service/src/workflow/workflow.service.ts (1)
1926-1926: LGTM! Document validation is more robust.
The changes improve the handling of document validation by properly handling null cases and improving type safety.
Let's verify the document validation logic:
✅ Verification successful
Document validation changes are properly implemented and consistent
The codebase search reveals that:
- The document validation logic is consistently handling null/undefined cases using optional chaining across multiple locations
- The implementation properly strips out the
propertiesSchemaproperty during validation - Similar null-safe patterns are used throughout the service for document handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify document validation implementation
# Check for potential edge cases in document validation
ast-grep --pattern 'documents: (context?.documents || []).map' | \
while read -r line; do
echo "Found document validation pattern. Checking for test coverage..."
rg -q "context: { documents: \[\] }" services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts && \
echo "Empty documents array test case found" || \
echo "Warning: Missing test case for empty documents array"
done
Length of output: 79
Script:
#!/bin/bash
# Let's verify the document validation implementation with a broader search
# Check the implementation context
rg -A 5 -B 5 "documents: \(context\?\.documents \|\| \[\]\)\.map" services/workflows-service/src/workflow/workflow.service.ts
# Check test coverage for document validation
rg -A 5 "documents:" services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
# Look for other document validation patterns
rg "context\?\.documents" services/workflows-service/src/workflow/workflow.service.ts
Length of output: 1212
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow.controller.external.ts
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
Dismissed
Show dismissed
Hide dismissed
- Update the assertion to match an object structure - Include additional expected properties in the response check (That test is so thorough, it could pass a background check for a secret agent)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts (6)
178-178: Typo in test descriptionThe word "recieving" is misspelled. The correct spelling is "receiving".
Please correct the typo in the test description:
-it('should return 401 when not recieving authorization token', async () => { +it('should return 401 when not receiving authorization token', async () => {
253-253: Typo in variable name 'NON_EXISTANT_WORKFLOW_ID'The word "NON_EXISTANT" is misspelled. The correct spelling is "NON_EXISTENT".
Please correct the variable name:
-const workflowId = 'NON_EXISTANT_WORKFLOW_ID'; +const workflowId = 'NON_EXISTENT_WORKFLOW_ID';
191-191: Avoid hard-coded credentials in test codeThe string
"Bearer INVALID_API_KEY"is hard-coded as an authorization header. While this is a test, it's good practice to define such values as constants or use a more generic placeholder.Consider defining a constant for invalid API keys or using a clearly invalid placeholder to improve readability.
- .set('authorization', 'Bearer INVALID_API_KEY') + .set('authorization', 'Bearer INVALID_API_KEY_PLACEHOLDER')
193-193: Inconsistent property name in payloadThe property
workflowDefinitionIdis used here, whereas other tests useworkflowId. For consistency and to avoid potential bugs, it's advisable to use the same property name across all tests.Update the property name to match the other test cases:
- workflowDefinitionId: 'test-id', + workflowId: 'test-id',
195-195: Inconsistent structure for 'context' propertyThe
contextobject usesentityIddirectly, while other tests nest it inside anentityobject. This inconsistency might lead to confusion or errors.Align the structure with other test cases for consistency:
- context: { entityId: 'test-entity' }, + context: { entity: { id: 'test-entity' } },
112-112: Avoid using '@ts-ignore' directiveThe
// @ts-ignoredirective suppresses TypeScript errors but can hide potential issues.Consider properly typing the
req.userproperty instead of ignoring TypeScript checks. You can extend theRequestinterface to include auserproperty:interface AuthenticatedRequest extends Request { user: { user: User; type: string; projectIds: string[]; }; } // Then use AuthenticatedRequest in the middleware const userAuthOverrideMiddleware = (req: AuthenticatedRequest, res: any, next: any) => { req.user = { user: assignee, type: 'user', projectIds: [project.id], }; next(); };
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
Outdated
Show resolved
Hide resolved
- Update test and exception messages to fix the spelling of "Definition" - Ensure consistency in error handling output across the service (your typos are so recurrent, they're basically the project's middle name)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts (2)
140-174: Consider extracting test data setup into helper functionsThe test data setup logic could be extracted into helper functions to improve readability and reusability.
// Example helper function: async function createTestWorkflowDefinition( repository: WorkflowDefinitionRepository, projectId: string, workflowId: string, ) { return repository.create({ data: { id: workflowId, name: 'workflow-name', definitionType: 'statechart-json', definition: {}, project: { connect: { id: projectId, }, }, }, }); }
178-178: Fix typo in test description"recieving" should be "receiving"
- it('should return 401 when not recieving authorization token', async () => { + it('should return 401 when not receiving authorization token', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.controller.external.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/workflow/workflow.controller.external.ts
🔇 Additional comments (3)
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts (3)
290-317: Enhance successful workflow creation test validation
While the test validates the basic response structure, consider adding more comprehensive validation of the created workflow.
55-55:
Uninitialized 'assignee' variable may cause issues
The variable assignee is declared but never initialized before being used in the middleware.
Also applies to: 112-112
191-191:
Hard-coded API key in test
Using hard-coded values for sensitive data like API keys is not recommended.
services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
Show resolved
Hide resolved
(your test suite is so long, I half expect it to come with a subscription plan)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation