chore(tests): fix SonarCloud test-assertion smells in integration tests#569
Closed
alichherawalla wants to merge 1 commit into
Closed
chore(tests): fix SonarCloud test-assertion smells in integration tests#569alichherawalla wants to merge 1 commit into
alichherawalla wants to merge 1 commit into
Conversation
S5906: replace generic assertions with dedicated matchers per Sonar's exact suggestions (toHaveLength / toBeNull / toBeGreaterThan / toBeLessThanOrEqual) across 36 integration test files. Every asserted value is unchanged — tests assert exactly the same things. S5976: parameterize the 3 near-identical <think>-tag stripping tests in imageGenerationFlow.test.ts into a single it.each table (coverage identical). Pure test-quality nits; no production code touched.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThis test-only change standardizes array and null assertions across integration suites. It replaces direct ChangesIntegration assertion modernization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/integration/generation/imageGenerationFlow.test.ts`:
- Around line 1183-1218: Delete the parameterized mockist test block around the
thinking-model prompt cases, including its mocked generateResponse setup and
toHaveBeenCalled assertion. Do not replace or repair it; follow the project
guideline that tests under __tests__ should use rendered integration behavior
and user-visible assertions instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b31c052-aa34-49d3-857d-cf844bd0a9fc
📒 Files selected for processing (36)
__tests__/integration/audio/voiceModeImageJourney.rendered.happy.test.tsx__tests__/integration/audio/voiceModeResendEnhancedImage.rendered.redflow.test.tsx__tests__/integration/audio/voiceModeResendImageRoutes.rendered.redflow.test.tsx__tests__/integration/chat/voiceNoteChatModeEmptyTurn.redflow.test.ts__tests__/integration/downloads/downloadedCountBadge.rendered.happy.test.tsx__tests__/integration/downloads/iosImageStagingPurgedRedownloads.redflow.test.ts__tests__/integration/downloads/iosImageStagingPurgedRedownloads.rendered.redflow.test.tsx__tests__/integration/downloads/iosInterruptedNoFailedEntry.redflow.test.ts__tests__/integration/downloads/iosTextRetryReissues.rendered.redflow.test.tsx__tests__/integration/downloads/queuedDownloadsSurviveKill.redflow.test.ts__tests__/integration/downloads/textRetryReissuesOnIosLostDownloadId.rendered.redflow.test.tsx__tests__/integration/generation/enhancementNoThinking.rendered.redflow.test.tsx__tests__/integration/generation/enhancementStreamingProgress.rendered.redflow.test.tsx__tests__/integration/generation/generationFlow.test.ts__tests__/integration/generation/imageGenerationFlow.test.ts__tests__/integration/generation/queuedForceImagePreservesMode.rendered.redflow.test.tsx__tests__/integration/generation/queuedSendFeedback.test.ts__tests__/integration/generation/reloadRaceKeepsThinking.rendered.redflow.test.tsx__tests__/integration/generation/remoteParallelTools.rendered.happy.test.tsx__tests__/integration/generation/resendImageRoutes.rendered.redflow.test.tsx__tests__/integration/generation/resendImageRoutesLlama.rendered.redflow.test.tsx__tests__/integration/generation/resendImageTurnFallsBackToText.rendered.redflow.test.tsx__tests__/integration/happy/imageBackends.happy.test.tsx__tests__/integration/happy/imageIntentRouting.happy.test.tsx__tests__/integration/happy/imageLightbox.happy.test.tsx__tests__/integration/happy/imageModeToggle.happy.test.tsx__tests__/integration/happy/imageOomCard.happy.test.tsx__tests__/integration/happy/smartBudgeting.happy.test.tsx__tests__/integration/happy/speakMessage.happy.test.tsx__tests__/integration/happy/tools.happy.test.tsx__tests__/integration/image/imageTunablesReadFreshFromStore.redflow.test.ts__tests__/integration/models/activeModelService.test.ts__tests__/integration/onboarding/spotlightFlowIntegration.test.ts__tests__/integration/rag/embeddingFlow.test.ts__tests__/integration/rag/ragFlow.test.ts__tests__/integration/stores/chatStoreIntegration.test.ts
Comment on lines
+1183
to
+1218
| it.each([ | ||
| { | ||
| name: 'should strip <think> tags from thinking model responses', | ||
| // Simulate a thinking model that wraps reasoning in <think> tags | ||
| inputPrompt: 'sunset over mountains', | ||
| enhancedResponse: | ||
| '<think>Let me enhance this prompt by adding artistic details...</think>A majestic sunset over mountains, golden hour lighting, oil painting style', | ||
| // The prompt passed to image generation should NOT contain <think> tags | ||
| expectedPrompt: 'A majestic sunset over mountains, golden hour lighting, oil painting style', | ||
| }, | ||
| { | ||
| name: 'should handle thinking model response that is only a think block', | ||
| // Simulate a model that only outputs thinking with no actual response | ||
| inputPrompt: 'a cat', | ||
| enhancedResponse: '<think>I need to think about how to enhance this prompt...</think>', | ||
| // When stripping produces empty string, should fall back to original prompt | ||
| expectedPrompt: 'a cat', | ||
| }, | ||
| { | ||
| name: 'should handle response without think tags normally', | ||
| // Non-thinking model returns plain enhanced prompt | ||
| inputPrompt: 'simple prompt', | ||
| enhancedResponse: 'A beautiful enhanced prompt with details', | ||
| expectedPrompt: 'A beautiful enhanced prompt with details', | ||
| }, | ||
| ])('$name', async ({ inputPrompt, enhancedResponse, expectedPrompt }) => { | ||
| setupThinkingModelEnhancement(); | ||
| // Simulate a thinking model that wraps reasoning in <think> tags | ||
| mockLlmService.generateResponse.mockResolvedValue( | ||
| '<think>Let me enhance this prompt by adding artistic details...</think>A majestic sunset over mountains, golden hour lighting, oil painting style' | ||
| ); | ||
|
|
||
| await imageGenerationService.generateImage({ | ||
| prompt: 'sunset over mountains', | ||
| }); | ||
|
|
||
| // The prompt passed to image generation should NOT contain <think> tags | ||
| expect(mockLocalDreamService.generateImage).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| prompt: 'A majestic sunset over mountains, golden hour lighting, oil painting style', | ||
| }), | ||
| expect.any(Function), | ||
| expect.any(Function), | ||
| ); | ||
| }); | ||
|
|
||
| it('should handle thinking model response that is only a think block', async () => { | ||
| setupThinkingModelEnhancement(); | ||
| // Simulate a model that only outputs thinking with no actual response | ||
| mockLlmService.generateResponse.mockResolvedValue( | ||
| '<think>I need to think about how to enhance this prompt...</think>' | ||
| ); | ||
|
|
||
| await imageGenerationService.generateImage({ | ||
| prompt: 'a cat', | ||
| }); | ||
|
|
||
| // When stripping produces empty string, should fall back to original prompt | ||
| expect(mockLocalDreamService.generateImage).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| prompt: 'a cat', | ||
| }), | ||
| expect.any(Function), | ||
| expect.any(Function), | ||
| ); | ||
| }); | ||
|
|
||
| it('should handle response without think tags normally', async () => { | ||
| setupThinkingModelEnhancement(); | ||
| // Non-thinking model returns plain enhanced prompt | ||
| mockLlmService.generateResponse.mockResolvedValue( | ||
| 'A beautiful enhanced prompt with details' | ||
| ); | ||
| mockLlmService.generateResponse.mockResolvedValue(enhancedResponse); | ||
|
|
||
| await imageGenerationService.generateImage({ | ||
| prompt: 'simple prompt', | ||
| prompt: inputPrompt, | ||
| }); | ||
|
|
||
| expect(mockLocalDreamService.generateImage).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| prompt: 'A beautiful enhanced prompt with details', | ||
| prompt: expectedPrompt, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Delete this mockist test rather than repairing it.
As per coding guidelines for __tests__/**/*.{ts,tsx}, you should delete mockist tests that mock application code or assert toHaveBeenCalled rather than repairing or parameterizing them. Tests should instead be rendered integration tests that mount the screen and assert what the user sees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/integration/generation/imageGenerationFlow.test.ts` around lines
1183 - 1218, Delete the parameterized mockist test block around the
thinking-model prompt cases, including its mocked generateResponse setup and
toHaveBeenCalled assertion. Do not replace or repair it; follow the project
guideline that tests under __tests__ should use rendered integration behavior
and user-visible assertions instead.
Source: Coding guidelines
26 tasks
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes SonarCloud code smells confined to test files under `tests/integration/`. Pure test-quality nits — no production code touched, no asserted value changed. Every test still asserts exactly the same thing.
What changed
S5906 (78 issues) — dedicated matchers over generic `.toBe()`
Applied Sonar's exact suggested replacement per line across 36 integration test files:
S5976 (1 issue) — parameterize duplicated tests
The 3 near-identical ``-tag stripping tests in `imageGenerationFlow.test.ts` collapsed into one `it.each` table. Each case keeps its descriptive name and coverage is identical.
Verification
Summary by CodeRabbit