Wip/farmaazon/chromatic in package tests #12338
Conversation
Frizi
requested review from
PabloBuchu,
indiv0,
somebody1234,
MrFlashAccount,
farmaazon,
vitvakatu,
kazcw,
AdRiley,
jdunkerley and
hubertp
as code owners
🧪 Storybook is successfully deployed!📊 Dashboard:
|
| /** | ||
| * Setup for all tests: checks if and where electron exec is. | ||
| * @throws when no Enso package could be found. | ||
| */ |
There was a problem hiding this comment.
Is this docs still relevant? To me all fields (testRunId, projectsDir) have something from being "setup"
| await page.mouse.move(0, 0) // Avoid showing a tooltip | ||
| await expect(page.locator('.GraphEditor .GraphNode.pending')).toHaveCount(0) |
There was a problem hiding this comment.
This check may pass before finishing execution, because we may run this check before the first "pending" updates arrive from the engine.
We may add expectation to have some node non-pending first, but I'm not sure how it would behave in really fast computations.
| 'Didn\'t see the visualization update after "Write once" action; assuming it\'s already done', | ||
| ) | ||
| } | ||
| await expect(writeNode.locator('.TableVisualization')).toContainText(OUTPUT_FILE) |
There was a problem hiding this comment.
Given the above, I still think this check is flaky and keep the try/catch.
| @@ -106,6 +119,9 @@ electronTest('Local Workflow', async ({ page, app, projectsDir }) => { | |||
| expect(projectFiles).toContain('images') | |||
| const images = await fs.readdir(pathModule.join(PROJECT_PATH, 'images')) | |||
| expect(images).toContain('image.png') | |||
| await expect(page.locator('.GraphEditor .GraphNode.pending')).toHaveCount(0) | |||
There was a problem hiding this comment.
This is a strange check - we don't touch code after last such check - if it's needed, please add info why.
| @@ -164,6 +164,9 @@ | |||
| "@types/ws": "*" | |||
| } | |||
| } | |||
| }, | |||
| "patchedDependencies": { | |||
| "@chromatic-com/playwright": "patches/@chromatic-com__playwright.patch" | |||
There was a problem hiding this comment.
Shouldn't we set exact version requirement on chromatic dependency if we patch it?
| const __browserFn = innerPage.context().browser | ||
| Object.defineProperty(innerPage.context(), 'browser', { | ||
| value: function browser() { | ||
| return ( | ||
| __browserFn.apply(this) ?? { | ||
| browserType: () => ({ | ||
| name: () => 'chromium', | ||
| }), | ||
| } | ||
| ) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
- Why double underscore?
- Why this is needed? Could you add a comment?
Pull Request Description
Fixes #11710
Improved visual stability of electron testing scenario, reworked the electron-based test definition to use API that's compatible with chromatic snapshots. Added upload of snapshots, though they are not yet verified as required for PRs due to still existing visual stability issues.