Fix E2E tests #907
Fix E2E tests #907
Conversation
WalkthroughAdded defensive error handling around the VS Code formatting command in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant formatDMC
participant VS Code
rect rgb(220, 240, 255)
note over Client,VS Code: Before (No Error Handling)
Client->>formatDMC: formatDMC()
formatDMC->>VS Code: executeFormatDocumentProvider()
alt Success
VS Code-->>formatDMC: TextEdit[]
formatDMC->>Client: Apply edits
else Exception
VS Code-->>formatDMC: Error
formatDMC-->>Client: ✗ Exception propagates
end
end
rect rgb(240, 255, 240)
note over Client,VS Code: After (With Error Handling)
Client->>formatDMC: formatDMC()
formatDMC->>VS Code: executeFormatDocumentProvider()
alt Success
VS Code-->>formatDMC: TextEdit[]
formatDMC->>Client: Apply edits
else Exception
VS Code-->>formatDMC: Error
formatDMC->>formatDMC: Log error message
formatDMC->>Client: Return early (silent)
end
end
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
01163b7 to
e41d094
Compare
![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
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
105-276: Consider documenting the retention of this unused function.Since the test registration on line 43 is commented out, this function is currently unused. If this is a temporary measure and the test will be re-enabled soon, consider adding a comment at the function definition explaining this. Otherwise, you might want to temporarily remove the function to reduce code clutter until it's ready to be used again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(1 hunks)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
Outdated
Show resolved
Hide resolved
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
e41d094 to
52f8001
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts (1)
298-298: Consider waiting for a specific condition instead of a fixed delay.Fixed delays like
waitForTimeout(2000)are generally discouraged in E2E tests because they:
- Add unnecessary wait time when the condition is satisfied earlier
- May still be flaky if 2000ms isn't always sufficient
- Don't clearly communicate what condition is being awaited
Investigate whether there's a specific condition you can wait for after filling the textarea. For example:
const textArea = this.sidePanel.locator(`textarea`); await textArea.waitFor(); await textArea.fill(content); -await page.page.waitForTimeout(2000); - const submitBtn = this.sidePanel.locator(`vscode-button:text("Save")`); await submitBtn.waitFor(); +await expect(submitBtn).toBeEnabled(); await submitBtn.click();Or if there's a loading indicator, validation message, or network request you can wait for:
// Wait for validation/processing to complete await this.sidePanel.locator('.validation-complete').waitFor(); // or await this.container.waitForLoadState('networkidle');Can you verify what specific condition needs to be satisfied before clicking Save? This would make the test more reliable and potentially faster.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
52f8001 to
5fba2fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts (2)
149-157: Consider removing debug logs or using a proper logging framework.The
console.errorcalls on lines 150 and 153 appear to be debug logging rather than actual error logging. While helpful for troubleshooting E2E tests, these should be removed before merging or replaced with a proper logging framework to avoid cluttering production console output.
158-162: Consider removing debug logs or using a proper logging framework.Similar to the previous segment, the
console.errorstatements on lines 158, 160, and 162 are debug logs tracking the execution flow. These should be cleaned up or moved to a proper logging framework before production deployment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(1 hunks)workspaces/mi/mi-extension/src/util/tsBuilder.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- workspaces/mi/mi-extension/src/util/tsBuilder.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts (2)
workspaces/mi/mi-extension/src/util/tsBuilder.ts (2)
updateTsFileIoTypes(35-114)updateTsFileCustomTypes(116-153)workspaces/mi/mi-extension/src/stateMachine.ts (1)
refreshUI(797-811)
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts (1)
164-168: Good error handling improvement.The enhanced error handling properly logs the error, displays a user-friendly message, and returns a failure response. This is a solid improvement to the error flow.
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
5fba2fb to
648d830
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts (1)
164-168: Consider providing more specific error details to users.The error handling correctly logs and returns
{ success: false }, but the user-facing error message is generic. Whileconsole.error(error)helps developers debug, users only see "Error while updating DMC file." without understanding what went wrong.Consider including error context in the user message (while being careful not to expose sensitive information):
} catch (error: any) { console.error(error); - window.showErrorMessage("Error while updating DMC file."); + window.showErrorMessage(`Error while updating DMC file: ${error?.message || 'Unknown error'}`); return resolve({ success: false }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(1 hunks)workspaces/mi/mi-extension/src/util/tsBuilder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- workspaces/mi/mi-extension/src/util/tsBuilder.ts
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts (2)
workspaces/mi/mi-extension/src/util/tsBuilder.ts (2)
updateTsFileIoTypes(35-114)updateTsFileCustomTypes(116-153)workspaces/mi/mi-extension/src/stateMachine.ts (1)
refreshUI(797-811)
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts
Outdated
Show resolved
Hide resolved
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
648d830 to
0193590
Compare
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
0193590 to
0359e39
Compare
2 participants
$subject
Summary by CodeRabbit