Conversation
WalkthroughDisabled/removed portions of an E2E custom-function mapping test (commented/removed interactions and cleanup), adjusted mapping fixture output fields and property order, added debug logging to file comparisons in the DataMapper helper, and made the Trivy CI step non-fatal by treating its exit code as 0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant E2E as E2E Test
participant UI as DataMapper UI
participant Fixture as Mapping Fixture
participant Helper as DataMapper.compareFiles
rect rgba(255,244,230,0.9)
Note over E2E,UI: Custom-function UI mapping sequence removed/commented
E2E ->> UI: (was) trigger custom-function mapping interactions
UI --x E2E: interaction skipped / commented out
end
rect rgba(232,245,233,0.9)
Note over E2E,Helper: File comparison now logs contents
E2E ->> Helper: compareFiles(fileA, fileB)
Helper ->> Helper: console.debug(fileAContent, fileBContent)
Helper -->> E2E: comparison result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
214-214: Consistent timeout increase appropriately addresses test flakiness for focus state transitions.The explicit 10-second timeout is consistently applied to all assertions verifying the expression bar loses focus after canvas interactions. This aligns with Playwright best practices for handling async UI state changes and is a reasonable extension of the 5-second default timeout.
Consider extracting the timeout to a named constant for improved maintainability:
+const EXPRESSION_BAR_UNFOCUS_TIMEOUT = 10000;Then apply across all four instances (lines 214, 223, 327, 453):
-await expect(expressionBar).not.toBeFocused({ timeout: 10000 }); +await expect(expressionBar).not.toBeFocused({ timeout: EXPRESSION_BAR_UNFOCUS_TIMEOUT });
📜 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(4 hunks)
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
c79c1f2 to
05ad50d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-extension/package.json(2 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
2 times, most recently
from
1d2574d to
e280256
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
214-217: Consider extracting the focus-wait pattern into a helper method.The same focus-wait pattern appears in four locations. Extracting it into a reusable helper method would reduce duplication and make future maintenance easier.
Example helper method to add to the test file or DataMapper component class:
async function waitForExpressionBarBlur(dmWebView, expressionBar: Locator, timeoutMs: number = 30000) { await dmWebView.waitForFunction( (el) => el !== document.activeElement, await expressionBar.elementHandle(), { timeout: timeoutMs } ); await expect(expressionBar).not.toBeFocused(); }Then replace each occurrence with:
await waitForExpressionBarBlur(dmWebView, expressionBar);Also applies to: 226-229, 333-336, 462-465
📜 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(4 hunks)
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
214-217: LGTM! Explicit focus waits improve test stability.The added
waitForFunctioncalls explicitly wait for the expression bar to lose focus before asserting, which addresses potential flakiness in the E2E tests. The 30-second timeout is conservative but appropriate for extension tests that may experience delays in CI environments.
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
4 times, most recently
from
8bfbd3f to
21172fe
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)
136-141: Remove commented-out code.The old implementation is preserved in version control, so there's no need to keep it as a comment. Removing it improves code readability.
Apply this diff:
- // public async waitForOutline(locator: Locator, timeout = 5000) { - // await expect(async () => { - // const outline = await locator.evaluate(el => window.getComputedStyle(el).outline); - // expect(outline && !outline.includes('none')).toBeTruthy(); - // }).toPass({ timeout }); - // } -
📜 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)
🔇 Additional comments (2)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts (1)
143-154: Improved outline validation logic.The new implementation is more robust than string parsing. Separately checking
outlineStyleandoutlineWidthprovides clearer validation that an outline is actually visible.workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
223-223: LGTM! Explicit blur improves test reliability.Explicitly calling
blur()ensures the expression bar loses focus deterministically, reducing potential flakiness in the test. This complements the more robust outline validation in DataMapper.ts.
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
2 times, most recently
from
78f2153 to
8fc9326
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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)
🔇 Additional comments (2)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts (1)
143-161: Improved focus outline detection.The enhanced implementation properly validates all outline properties (style, width, color) and also detects focus via
box-shadow, which some platforms use as a focus indicator. This is more robust than the previous string-based check and should improve cross-platform test reliability.workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
222-224: Inconsistency confirmed: explicit blur() at line 223 differs from earlier pattern.The inconsistency is verified. Line 212–214 uses only a canvas click to remove focus, while line 222–224 adds an explicit
evaluate().blur()call before the same assertion. Both patterns have identical assertion intent (not.toBeFocused()).This pattern difference could indicate:
- The earlier pattern (line 212–214) is insufficient and should also use explicit blur
- The explicit blur at line 223 is defensive for timing sensitivity
- Different contexts justify different approaches
Since git history is unavailable and the sandbox cannot execute tests, manual verification is needed: run the test multiple times without the explicit blur at line 223 to confirm whether it's addressing a genuine race condition or is optional defensive coding. If necessary, apply the same pattern to line 212–214 for consistency.
| // public async waitForOutline(locator: Locator, timeout = 5000) { | ||
| // await expect(async () => { | ||
| // const outline = await locator.evaluate(el => window.getComputedStyle(el).outline); | ||
| // expect(outline && !outline.includes('none')).toBeTruthy(); | ||
| // }).toPass({ timeout }); | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The old implementation should be deleted rather than commented out. Version control preserves the history if needed.
- // public async waitForOutline(locator: Locator, timeout = 5000) {
- // await expect(async () => {
- // const outline = await locator.evaluate(el => window.getComputedStyle(el).outline);
- // expect(outline && !outline.includes('none')).toBeTruthy();
- // }).toPass({ timeout });
- // }
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // public async waitForOutline(locator: Locator, timeout = 5000) { | |
| // await expect(async () => { | |
| // const outline = await locator.evaluate(el => window.getComputedStyle(el).outline); | |
| // expect(outline && !outline.includes('none')).toBeTruthy(); | |
| // }).toPass({ timeout }); | |
| // } |
🤖 Prompt for AI Agents
In
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts
around lines 136 to 141, there is a commented-out waitForOutline function block
that should be removed; delete the entire commented code (all lines of the
commented function) so the file contains no dead/commented-out implementation,
relying on version control history if the old code needs to be recovered.
fb4267a to
3a755fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
217-227: Critical: Security scanner made non-fatal.Changing
exit-codefrom'1'to'0'causes Trivy to never fail the build, even when vulnerabilities are detected. This defeats the purpose of vulnerability scanning and allows insecure code to be merged.If you're experiencing transient scanner failures, consider alternative solutions:
- Use
continue-on-error: truefor specific conditions- Filter specific vulnerabilities with
trivyignores- Adjust severity thresholds with
severity: 'CRITICAL,HIGH'Apply this diff to restore security enforcement:
- exit-code: '0' + exit-code: '1'
♻️ Duplicate comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts (1)
136-141: Remove commented-out code.This commented-out implementation should be deleted. Version control preserves the history if needed.
Apply this diff:
- // public async waitForOutline(locator: Locator, timeout = 5000) { - // await expect(async () => { - // const outline = await locator.evaluate(el => window.getComputedStyle(el).outline); - // expect(outline && !outline.includes('none')).toBeTruthy(); - // }).toPass({ timeout }); - // } -
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts (1)
222-228: Reconsider the double-click focus pattern.This helper uses force clicks and a double-click pattern to ensure focus, which may cause unintended side effects:
- Double-clicking could trigger unexpected behaviors in the UI
- The 50ms delay is a magic number without clear justification
- Force clicks bypass Playwright's actionability checks
If focus is this unreliable, consider investigating why VS Code Webview focus handling is problematic rather than working around it.
A simpler approach:
private async forceFocus(locator: Locator) { - // Click twice with a short delay to ensure real focus - await locator.click({ force: true }); + await locator.click(); await locator.evaluate(el => el.focus()); - await locator.click({ force: true }); - await this._page.waitForTimeout(50); // tiny stabilization delay + await this._page.waitForTimeout(100); // allow focus to settle }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts(2 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts (1)
143-161: LGTM: Robust focus detection.The enhanced
waitForOutlinecorrectly handles both CSS outline and box-shadow focus indicators, improving cross-platform reliability.
| // wait for both fields to exist and be visible | ||
| await sourceField.waitFor({ state: 'visible' }); | ||
| await targetField.waitFor({ state: 'visible' }); | ||
|
|
||
| // Scroll both into view to avoid click misfires in WebViews | ||
| await sourceField.scrollIntoViewIfNeeded(); | ||
| await targetField.scrollIntoViewIfNeeded(); | ||
|
|
||
| // --- Improve focus reliability --- | ||
| // Sometimes one click isn't enough in VS Code Webview | ||
| await this.forceFocus(sourceField); | ||
|
|
||
| await sourceField.click({force: true}); | ||
| await this.waitForOutline(sourceField, 30000); | ||
| await targetField.click({force: true}); | ||
| // --- Ensure outline is visible (even on Windows) --- | ||
| await this.waitForOutline(sourceField, 30000); | ||
|
|
||
| // Now click target | ||
| await targetField.scrollIntoViewIfNeeded(); | ||
| await targetField.click({ force: true }); | ||
|
|
There was a problem hiding this comment.
Reduce timeout and avoid force clicks.
Several concerns:
-
30-second timeout (line 181): This is excessive and will significantly slow tests. If focus takes 30 seconds to appear, there's a deeper issue. Consider 5-10 seconds maximum.
-
Force click (line 185): Using
{ force: true }bypasses Playwright's actionability checks, potentially masking real issues where the target isn't actually clickable in production. -
The overall pattern suggests working around timing/focus issues rather than addressing root causes.
Consider this approach:
- // --- Ensure outline is visible (even on Windows) ---
- await this.waitForOutline(sourceField, 30000);
+ // Ensure outline is visible
+ await this.waitForOutline(sourceField, 10000);
// Now click target
await targetField.scrollIntoViewIfNeeded();
- await targetField.click({ force: true });
+ await targetField.click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // wait for both fields to exist and be visible | |
| await sourceField.waitFor({ state: 'visible' }); | |
| await targetField.waitFor({ state: 'visible' }); | |
| // Scroll both into view to avoid click misfires in WebViews | |
| await sourceField.scrollIntoViewIfNeeded(); | |
| await targetField.scrollIntoViewIfNeeded(); | |
| // --- Improve focus reliability --- | |
| // Sometimes one click isn't enough in VS Code Webview | |
| await this.forceFocus(sourceField); | |
| await sourceField.click({force: true}); | |
| await this.waitForOutline(sourceField, 30000); | |
| await targetField.click({force: true}); | |
| // --- Ensure outline is visible (even on Windows) --- | |
| await this.waitForOutline(sourceField, 30000); | |
| // Now click target | |
| await targetField.scrollIntoViewIfNeeded(); | |
| await targetField.click({ force: true }); | |
| // wait for both fields to exist and be visible | |
| await sourceField.waitFor({ state: 'visible' }); | |
| await targetField.waitFor({ state: 'visible' }); | |
| // Scroll both into view to avoid click misfires in WebViews | |
| await sourceField.scrollIntoViewIfNeeded(); | |
| await targetField.scrollIntoViewIfNeeded(); | |
| // --- Improve focus reliability --- | |
| // Sometimes one click isn't enough in VS Code Webview | |
| await this.forceFocus(sourceField); | |
| // Ensure outline is visible | |
| await this.waitForOutline(sourceField, 10000); | |
| // Now click target | |
| await targetField.scrollIntoViewIfNeeded(); | |
| await targetField.click(); |
🤖 Prompt for AI Agents
In
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts
around lines 168-186, replace the 30s waitForOutline timeout with a much shorter
5-10s timeout (e.g., 5s) and remove the use of targetField.click({ force: true
}); instead wait for the target to be actionable (visible, enabled, and stable)
using Playwright waitFor / waitForElementState calls and then perform a normal
click; if focus is flaky, retry focus+click a couple of times with short backoff
(e.g., focus(), wait 100-200ms, click()) rather than forcing the click so tests
fail when the UI is not actually clickable.
7bb3323 to
ee951c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
193-209: Custom function mapping scenario and TS verification look solid; tab locator could be made more robustThe new
iCustomFn → oCustomFnmapping plus the three link assertions give good coverage of the o2o‑func behavior, and explicitly closing the${dmName}.tstab afterverifyTsFileContentshould help keep later steps isolated.One small robustness suggestion: the tab locator
getByRole('tab', { name: \${dmName}.ts, Editor Group` })` is tightly coupled to VS Code’s ARIA label format and could break if the label changes (e.g., dirty markers or different suffix). Using a regex/startsWith‑style name instead would make this less brittle, for example:- const editorTab = page.page.getByRole('tab', { name: `${dmName}.ts, Editor Group` }); + const editorTab = page.page.getByRole('tab', { name: new RegExp(`^${dmName}\\.ts`) });You could also optionally standardize on either raw
'basic/map.ts'orpath.join('basic', 'map.ts')inverifyTsFileContentcalls for consistency, but the current code should work as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(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(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build.yml
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/components/DataMapper.ts
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
235-241: Explicit blur on the expression editor should reduce focus flakinessAdding
await expressionBar.evaluate((el: HTMLElement) => el.blur());after clicking the canvas provides a deterministic way to drop focus before assertingnot.toBeFocused(), which should help stabilize this flow given the earlier “Fails here” comment in nearby code. The PlaywrightLocator.evaluateusage and async sequencing look correct.
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
2 times, most recently
from
4f31920 to
84a0ea8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(3 hunks)
🔇 Additional comments (3)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (3)
193-206: LGTM! Custom function mapping test looks solid.The test correctly exercises the custom function mapping flow: mapping fields, verifying intermediate ports and connectors, and handling the editor tab lifecycle. The explicit waits for 'attached' and 'detached' states ensure proper synchronization.
Minor observation: The editor tab is closed immediately without verifying its content. If the editor is expected to display specific mapping code, consider adding a content check before closing to strengthen the test coverage.
237-237: Good addition for test stability.Explicitly calling
blur()ensures the expression bar loses focus before the subsequent assertion, reducing potential flakiness from browser timing variations.
263-263: LGTM! Consistent with earlier usage.Using
.first()is appropriate here and matches the pattern established at line 148 for the same test ID. This ensures the test targets a specific element when multiple intermediate ports exist.
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
Show resolved
Hide resolved
Added console logs to display contents of compared files.
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)
256-259: Remove debug console.log statements before merging.These console.log statements will clutter test output with full file contents on every comparison. If debugging is needed, consider using a conditional debug flag or Playwright's built-in
page.on('console')capture. For production test code, remove these entirely.Apply this diff to remove the debug statements:
public compareFiles(file1: string, file2: string) { const file1Content = fs.readFileSync(file1, 'utf8').replace(/\r\n/g, '\n'); const file2Content = fs.readFileSync(file2, 'utf8').replace(/\r\n/g, '\n'); - console.log("File1:"); - console.log(file1Content); - console.log("File2:"); - console.log(file2Content); return file1Content === file2Content; }
📜 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/components/DataMapper.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
248-272: Re-check delete expectations vsbasic/del.tsand.first()vs.nth(1)connector targetingCommenting out the deletions for
loc2(Line 250–Line 261 scope) andloc4(Line 269–Line 270) means the test no longer removes theoManyOneandoExpmappings, while the final assertion still compares againstbasic/del.ts(Line 272). Earlier feedback noted thatbasic/del.tsonly hadoManyOneErrand notoManyOne, so there’s a risk that the fixture and these steps are still out of sync.Also, for the many-one error case you now delete via:
const loc3_ = ...getByTestId('link-from-input.iManyOne3.OUT-to-datamapper-intermediate-port').first();(Line 263)- and a sub-link label using
.first()(Line 265),whereas the error-link assertions earlier rely on
.nth(1)to target the error connectors. That suggests.first()here may be deleting the non-error mapping instead of the error mapping, depending on connector ordering.Please verify which mappings
basic/del.tsexpects to be present/absent and then either:
- Re-enable the
loc2/loc4deletions, or- Update
basic/del.ts(and, if needed, the.first()/.nth(...)usage) so that the fixture exactly matches what this sequence produces.For convenience, you can re-check the fixture with something like:
#!/bin/bash # Inspect the basic delete fixture for many-one and expression-bar outputs grep -nE 'oManyOne|oManyOneErr|oExp' \ workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
193-206: Custom function mapping flow looks good; consider loosening the tab locatorThe custom-function mapping plus connector assertions are clear, and explicitly closing the
${dmName}.tseditor tab is a nice way to finish the flow. The one fragile piece is the hard-coded accessible name'${dmName}.ts, Editor Group', which may change with VS Code UI tweaks or localization.You could make this locator more robust by just anchoring on the file name and letting the rest of the label vary, e.g.:
- const editorTab = page.page.getByRole('tab', { name: `${dmName}.ts, Editor Group` }); + const editorTab = page.page.getByRole('tab', { name: new RegExp(`^${dmName}\\.ts`) });
📜 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(3 hunks)
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
232-238: Explicit blur on the expression bar should help stabilize focus behaviorAdding
await expressionBar.evaluate((el: HTMLElement) => el.blur());after clicking the canvas makes the loss-of-focus explicit and should reduce flakiness around the subsequentnot.toBeFocused()assertion, assuming the element is still attached at this point. No issues from a test-logic perspective.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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(3 hunks)
🔇 Additional comments (3)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (3)
193-206: Custom function test addition looks good.The test properly validates custom function mapping with appropriate waits for UI elements and cleans up the editor tab. The placement consolidates the test flow as intended.
237-237: Good addition for test stability.The explicit blur call ensures focus is properly lost before assertions, improving test reliability.
263-265: Selector stabilization improves test reliability.Using
nth(1)properly handles multiple matching elements and prevents selector ambiguity.
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(4 hunks)
🔇 Additional comments (2)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (2)
193-206: LGTM! Custom function mapping test properly structured.The new custom function mapping test follows best practices with proper waits for all UI elements (intermediate port links, connector nodes, editor tab) before proceeding to the next step.
237-237: Good practice: explicit blur for test stability.The explicit
blur()call after clicking the canvas ensures deterministic focus state transitions, which helps prevent flaky tests when native focus management is inconsistent.
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
Outdated
Show resolved
Hide resolved
Increased wait time for page timeout during tests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
260-272: Replace fixed 20s sleeps with condition-based waits for deletes.The new
waitForTimeout(20000)calls after deletingloc2andloc4reduce flakiness but still rely on arbitrary sleeps and keep the waits forstate: 'detached'commented out. This keeps the test non-deterministic and adds up to 40s of idle time in this block. The newloc3_with.first()and the targeted sub-link trash click are good stabilizations, though.If possible, prefer a condition-based wait tied to actual UI or file-state changes, for example:
- A locator-based wait on whatever does change reliably when the mapping is deleted (if
loc2/loc4never detach, wait on another selector instead of sleeping), or- Polling
dm.verifyTsFileContent('basic/del.ts')with a timeout rather than sleeping and then asserting once.This will make the test both faster and more deterministic than fixed 20s timeouts.
Also applies to: 264-266
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
237-237: Type annotation onelis optional here.The explicit
HTMLElementannotation isn’t required;evaluatecan infer the element type. You could simplify to avoid depending on DOM typings in the test TS config:- await expressionBar.evaluate((el: HTMLElement) => el.blur()); + await expressionBar.evaluate(el => el.blur());
📜 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(3 hunks)
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
193-205: Custom function mapping flow and editor-tab handling look consistent.The new custom function mapping (including connector waits) plus the explicit tab close on
${dmName}.tsis coherent with the rest of the test and should help keep focus in the data-mapper view stable before the later expression-bar steps.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
260-272: Fixloc4undefined reference error at line 270–272; remove dead commented timeouts.The
loc4variable declaration at line 222 is commented out as part of the disabled expression-bar block, but lines 270–272 reference it uncommented. This will throw aReferenceErrorat runtime. Either uncomment and re-enable theloc4declaration with its expression-bar mapping steps (lines 207–245), or comment out theloc4deletion block (lines 270–272) to match the disabled state above it.Also remove the dead
// await page.page.waitForTimeout(20000);comments at lines 262 and 272 or replace them with explicittimeoutparameters on the relevantwaitFor()calls if longer waits are needed.await loc2.locator('.codicon-trash').click({ force: true }); - // await page.page.waitForTimeout(20000); await loc2.waitFor({ state: 'detached' }); - await loc4.locator('.codicon-trash').click({ force: true }); - // await page.page.waitForTimeout(20000); - await loc4.waitFor({ state: 'detached' }); + // Expression-bar-based mapping (oExp) is currently disabled above, + // so we skip deleting it here. If oExp is re-enabled, restore this block. + // await loc4.locator('.codicon-trash').click({ force: true }); + // await loc4.waitFor({ state: 'detached' });
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
207-245: Expression-bar and expression-editor coverage is fully disabled; clarify and track intent.This block now comments out all expression-bar/editor interactions and the
basic/map.tscontent assertion, sotestBasicMappingsno longer validates expression-bar behaviour or its generated TS output. If this is a temporary workaround for flakiness, it’d be better to (a) add a clear comment with a linked issue, or (b) move this logic into a dedicated test markedtest.skip/test.fixmeinstead of leaving a large commented block.A minimal clarification patch could look like:
- // console.log('- Test expression bar'); + // Expression bar tests are temporarily disabled (see <ticket/issue-id>). + // TODO: Re-enable these steps once the flakiness is resolved. + // console.log('- Test expression bar');
📜 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/data/datamapper-files/basic/map.ts(0 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(4 hunks)
💤 Files with no reviewable changes (1)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/map.ts
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
193-200: Custom-function mapping step looks consistent.The custom-function mapping and surrounding logging follow the same pattern as other mapping tests and use the existing helpers correctly. No issues from a test-logic perspective.
ChinthakaJ98
force-pushed
the
e2e-test-fixes-01
branch
from
f925a5c to
5721483
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts (1)
230-244: Add TODO comment explaining why custom function test is disabled.The custom function mapping test is commented out rather than removed, suggesting this is temporary. However, there's no explanation of why it's disabled or when it should be re-enabled.
Given the PR title mentions "Increase assertion timeout," consider either:
- Adding a TODO comment explaining the issue and linking to a tracking ticket
- Actually increasing the timeout if that's the root cause
Apply this diff to document the temporary state:
+ // TODO: Custom function test temporarily disabled due to flakiness + // Track re-enablement in issue #XXX // console.log('- Test custom function'); // custom function mapping // objectOutput.oCustomFn = input.iCustomFn;workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts (1)
64-73: Type mismatch: mapFunction signature and return object are inconsistent.The function signature declares it returns
OutputRoot(line 64), but the interfaceOutputRootdefinesoCustomFnandoExpas required properties (lines 51-56), which are not included in the returned object. This would normally be a TypeScript compilation error.However, since this is a test fixture file used for text comparison (verified at dataMapper.spec.ts line 272), this inconsistency may be acceptable. The change correctly aligns with the disabled custom function test in the spec file.
If this fixture is meant to be valid TypeScript, consider making the omitted properties optional in the interface:
oObjProp: { p1: string p2: number } - oCustomFn: { + oCustomFn?: { k1: string k3: number } - oExp: string + oExp?: string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/map.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/map.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/map.ts (2)
51-54: TypeScript interface mismatch: oCustomFn declared but never assigned.The
OutputRootinterface declaresoCustomFn(lines 51-54), butmapFunctiondoes not assign it in the return statement (lines 64-78). This will result in the field beingundefinedat runtime, which may cause type errors or unexpected behavior downstream.Since the custom function test is commented out in
dataMapper.spec.ts(lines 230-243), either:
- Remove the
oCustomFnfield from theOutputRootinterface to match the implementation, or- Uncomment the test and restore the assignment:
oCustomFn: mapObjectToObject(input.iCustomFn),</invoke_comment_end -->
Also applies to: 64-78
80-82: Remove unused helper function.The
mapObjectToObjectfunction is defined but never invoked. This is dead code that should be removed to improve maintainability.Apply this diff to remove the unused function:
-function mapObjectToObject(iCustomFn: Root["iCustomFn"]): OutputRoot["oCustomFn"] { - return {}; -}</invoke_comment_end -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/map.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts
| // console.log('- Test custom function'); | ||
| // custom function mapping | ||
| // objectOutput.oCustomFn = input.iCustomFn; | ||
| await dm.mapFields('input.iCustomFn', 'objectOutput.oCustomFn', 'menu-item-o2o-func'); | ||
| // await dm.mapFields('input.iCustomFn', 'objectOutput.oCustomFn', 'menu-item-o2o-func'); | ||
|
|
||
| await dmWebView.getByTestId('link-from-input.iCustomFn.OUT-to-datamapper-intermediate-port').waitFor({ state: 'attached' }); | ||
| await dmWebView.getByTestId('link-from-datamapper-intermediate-port-to-objectOutput.oCustomFn.IN').waitFor({ state: 'attached' }); | ||
| await dmWebView.getByTestId('link-connector-node-objectOutput.oCustomFn.IN').waitFor(); | ||
| // await dmWebView.getByTestId('link-from-input.iCustomFn.OUT-to-datamapper-intermediate-port').waitFor({ state: 'attached' }); | ||
| // await dmWebView.getByTestId('link-from-datamapper-intermediate-port-to-objectOutput.oCustomFn.IN').waitFor({ state: 'attached' }); | ||
| // await dmWebView.getByTestId('link-connector-node-objectOutput.oCustomFn.IN').waitFor(); | ||
|
|
||
| const editorTab = page.page.getByRole('tab', { name: `${dmName}.ts, Editor Group` }); | ||
| await editorTab.waitFor({ state: 'attached' }); | ||
| // const editorTab = page.page.getByRole('tab', { name: `${dmName}.ts, Editor Group` }); | ||
| // await editorTab.waitFor({ state: 'attached' }); | ||
|
|
||
| await editorTab.locator('.codicon-close').click(); | ||
| await editorTab.waitFor({ state: 'detached' }); | ||
| // await editorTab.locator('.codicon-close').click(); | ||
| // await editorTab.waitFor({ state: 'detached' }); |
There was a problem hiding this comment.
PR objective mismatch: test steps disabled instead of timeout increased.
The PR title states "Increase assertion timeout," but these changes disable the custom function mapping test by commenting out the execution steps rather than adjusting timeout values.
If the goal is to address test flakiness or timing issues as implied by the title, consider:
- Uncommenting these steps and increasing the
timeoutparameter in the relevantwaitFor()calls (e.g.,waitFor({ state: 'attached', timeout: 10000 })), or - Updating the PR title and description to reflect that this change intentionally disables the custom function test path.
</invoke_comment_end -->
🤖 Prompt for AI Agents
In workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
around lines 230-243, the custom function mapping test steps were commented out
instead of increasing assertion timeouts; restore the commented test steps
(uncomment the mapping and all subsequent waitFor()/locator/editor interactions)
and add explicit timeout options to the relevant waitFor() calls (e.g.,
waitFor({ state: 'attached', timeout: 10000 }) or a suitable value for your CI),
ensuring all awaited locators and editor interactions include timeouts; if the
change was intentional to disable the test, update the PR title/description to
reflect that instead of claiming an "Increase assertion timeout" fix.
| // console.log('- Test custom function'); | ||
| // custom function mapping | ||
| // objectOutput.oCustomFn = input.iCustomFn; | ||
| await dm.mapFields('input.iCustomFn', 'objectOutput.oCustomFn', 'menu-item-o2o-func'); | ||
| // await dm.mapFields('input.iCustomFn', 'objectOutput.oCustomFn', 'menu-item-o2o-func'); | ||
|
|
||
| await dmWebView.getByTestId('link-from-input.iCustomFn.OUT-to-datamapper-intermediate-port').waitFor({ state: 'attached' }); | ||
| await dmWebView.getByTestId('link-from-datamapper-intermediate-port-to-objectOutput.oCustomFn.IN').waitFor({ state: 'attached' }); | ||
| await dmWebView.getByTestId('link-connector-node-objectOutput.oCustomFn.IN').waitFor(); | ||
| // await dmWebView.getByTestId('link-from-input.iCustomFn.OUT-to-datamapper-intermediate-port').waitFor({ state: 'attached' }); | ||
| // await dmWebView.getByTestId('link-from-datamapper-intermediate-port-to-objectOutput.oCustomFn.IN').waitFor({ state: 'attached' }); | ||
| // await dmWebView.getByTestId('link-connector-node-objectOutput.oCustomFn.IN').waitFor(); | ||
|
|
||
| const editorTab = page.page.getByRole('tab', { name: `${dmName}.ts, Editor Group` }); | ||
| await editorTab.waitFor({ state: 'attached' }); | ||
| // const editorTab = page.page.getByRole('tab', { name: `${dmName}.ts, Editor Group` }); | ||
| // await editorTab.waitFor({ state: 'attached' }); | ||
|
|
||
| await editorTab.locator('.codicon-close').click(); | ||
| await editorTab.waitFor({ state: 'detached' }); | ||
| // await editorTab.locator('.codicon-close').click(); | ||
| // await editorTab.waitFor({ state: 'detached' }); | ||
|
|
||
| expect(dm.verifyTsFileContent(path.join('basic', 'map.ts'))).toBeTruthy(); |
There was a problem hiding this comment.
Test execution and verification are inconsistent.
The custom function mapping steps are commented out (lines 230-243), meaning oCustomFn is never created in the data mapper. However, the test still verifies the TypeScript file content at line 245 against 'basic/map.ts'.
This creates ambiguity:
- If the test fixture
'basic/map.ts'includes theoCustomFnassignment, the verification will fail because the mapping was never created. - If the test fixture was updated to exclude
oCustomFn(as seen in the relatedmap.tsfile under review), then the commented test steps are obsolete and should be removed entirely rather than left as comments.
Either uncomment the test steps to execute the full custom function flow, or remove the commented code blocks and update documentation to reflect that custom function mapping is no longer tested in this suite.
</invoke_comment_end -->
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts (1)
79-81: Incomplete implementation ofmapObjectToObject.The function returns an empty object
{}but should return an object matchingOutputRoot["oCustomFn"]:{k1: string, k3: number}. This will cause the mapping to produce incorrect results and violate the output interface contract.Based on the PR context mentioning "Disabled/removed portions of an E2E custom-function mapping test," this may be intentionally stubbed. However, for production test data, implement the proper mapping logic.
Apply this diff to implement a basic mapping:
function mapObjectToObject(iCustomFn: Root["iCustomFn"]): OutputRoot["oCustomFn"] { - return {}; + return { + k1: iCustomFn.k1, + k3: iCustomFn.k2 // Or apply appropriate transformation + }; }</review_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/map.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/map.ts
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
| oManyOne: input.iManyOne1 + input.iManyOne2 + input.iManyOne3, | ||
| oManyOneErr: input.iManyOne2 + input.iManyOneErr + input.iManyOne3, |
There was a problem hiding this comment.
Type mismatch in oManyOneErr computation.
Line 67 concatenates iManyOne2 (string) + iManyOneErr (boolean) + iManyOne3 (string), which produces a string (e.g., "value1truevalue2"), but the OutputRoot interface expects oManyOneErr to be a number (line 38). JavaScript's + operator will coerce the boolean to a string rather than perform numeric addition.
If this is intentional test data for error scenarios (suggested by the "Err" suffix), consider documenting the expected behavior or explicitly handling type conversion.
</review_comment_end>
🤖 Prompt for AI Agents
In
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts
around lines 66-67, the oManyOneErr field is computed as input.iManyOne2 +
input.iManyOneErr + input.iManyOne3 which performs string concatenation because
iManyOne2 and iManyOne3 are strings and iManyOneErr is boolean; OutputRoot
expects oManyOneErr to be a number. Fix by converting operands to numbers (e.g.,
Number(input.iManyOne2) + Number(input.iManyOneErr) + Number(input.iManyOne3))
or otherwise compute a numeric value (parseInt/parseFloat as appropriate), or if
the intention is to test a type-error case, add a clarifying comment and adjust
the OutputRoot type to string.
| oObjDirect: input.iObjDirect, | ||
| oObjDirectErr: input.iObjDirect, | ||
| oObjProp: { | ||
| p1: input.iObjDirect.d1 + "HI", | ||
| p2: input.iObjProp.op2 | ||
| }, |
There was a problem hiding this comment.
Type mismatches in object field mappings.
Two type safety issues:
-
Line 69:
oObjDirectErris assignedinput.iObjDirect(which hasd2: number), butOutputRoot["oObjDirectErr"]expectsd2: string(line 45). -
Line 72:
oObjProp.p2is assignedinput.iObjProp.op2(which isstringper line 21), but the interface expectsp2: number(line 49).
If these are intentional for error-scenario testing, consider adding comments or explicit type coercion to clarify intent.
</review_comment_end>
| oCustomFn: mapObjectToObject(input.iCustomFn), | ||
| oExp: dmUtils.toUppercase(input.iExp) |
There was a problem hiding this comment.
oCustomFn assignment relies on incomplete implementation.
Line 74 assigns the result of mapObjectToObject(input.iCustomFn) to oCustomFn, but the function returns an empty object {} (line 80), while OutputRoot["oCustomFn"] expects {k1: string, k3: number}. This will cause type violations and likely test failures.
See the comment on lines 79-81 for details.
</review_comment_end>
🤖 Prompt for AI Agents
In
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts
around lines 74-75 (implementation at ~79-81), the mapObjectToObject function
currently returns an empty object which violates OutputRoot["oCustomFn"] ({k1:
string, k3: number}); update the function to map fields from the input to the
expected output shape (for example set k1 = iCustomFn.k1 and derive k3 from
iCustomFn.k2 or appropriate logic), ensure proper typing/null checks so the
returned object exactly matches {k1: string, k3: number}, and run type
checks/tests to confirm compliance.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/data/datamapper-files/basic/del.ts(1 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-extension/src/test/e2e-playwright-tests/dataMapper.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T06:44:07.498Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 969
File: workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts:23-44
Timestamp: 2025-11-20T06:44:07.498Z
Learning: In the data mapper node-utils.ts, when `lastViewIndex == 0`, the code is at the root view and should NOT use `focusInputRootMap` for fallback input node resolution. The truthiness check on `lastViewIndex` is intentional to skip the fallback at the root level.
Applied to files:
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts
🔇 Additional comments (1)
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts (1)
66-66: LGTM!The
oManyOnecomputation correctly concatenates three string inputs to produce a string output, matching the expected type.
| return { | ||
| oManyOneErr: input.iManyOne2 + input.iManyOneErr, | ||
| oManyOne: input.iManyOne1 + input.iManyOne2 + input.iManyOne3, | ||
| oManyOneErr: input.iManyOne2 + input.iManyOneErr + input.iManyOne3, | ||
| oObjDirect: input.iObjDirect, | ||
| oObjDirectErr: input.iObjDirect, | ||
| oObjProp: { | ||
| p1: input.iObjDirect.d1 + "HI", | ||
| p2: input.iObjProp.op2 | ||
| }, | ||
| oCustomFn: mapObjectToObject(input.iCustomFn) | ||
| oCustomFn: mapObjectToObject(input.iCustomFn), | ||
| oExp: dmUtils.toUppercase(input.iExp) | ||
| } |
There was a problem hiding this comment.
Missing required fields in mapFunction return statement.
The return statement is missing two required fields from the OutputRoot interface:
oPrimDirect(expected type:string, line 35)oPrimDirectErr(expected type:number, line 36)
This will cause TypeScript compilation errors due to type-safety violations.
Apply this diff to add the missing fields:
export function mapFunction(input: Root): OutputRoot {
return {
+ oPrimDirect: input.iPrimDirect,
+ oPrimDirectErr: input.iPrimDirectErr,
oManyOne: input.iManyOne1 + input.iManyOne2 + input.iManyOne3,
oManyOneErr: input.iManyOne2 + input.iManyOneErr + input.iManyOne3,Note: oPrimDirectErr expects a number but iPrimDirectErr is a string (line 10). If this is intentional test data for error scenarios, either:
- Convert the input:
oPrimDirectErr: Number(input.iPrimDirectErr)orparseInt(input.iPrimDirectErr) - Update the interface type to match the intended behavior
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/data/datamapper-files/basic/del.ts
around lines 65 to 76, the mapFunction return is missing two required OutputRoot
fields: oPrimDirect (string) and oPrimDirectErr (number); add oPrimDirect:
input.iPrimDirect and add oPrimDirectErr: Number(input.iPrimDirectErr) (or
parseInt) to the returned object so types align, or if the string-to-number
conversion is unintended, update the OutputRoot/interface to accept a string
instead.
1 participant
$subject
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.