Wait for export button to make test more reliable#6143
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6143 +/- ##
=======================================
Coverage 85.37% 85.37%
=======================================
Files 110 110
Lines 44215 44215
=======================================
Hits 37750 37750
Misses 6465 6465
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pierremtb
left a comment
pierremtb
left a comment
There was a problem hiding this comment.
Looks good generally and frankly we need this sorted out. Noted down a few common fixtures you could use around the cmd bar that should make it more consistent with other tests (potentially better, maybe not haha!)
|
|
||
| // Click the export button | ||
| await exportButton.click() | ||
| await page.waitForTimeout(1_000) // wait for export options to be available |
There was a problem hiding this comment.
That's a little sus no? Considering that the options are all hardcoded there
modeling-app/src/lib/commandBarConfigs/modelingCommandConfig.ts
Lines 236 to 242 in bfdf8ba
There was a problem hiding this comment.
I think I'd prefer to see a cmdBar.expectState call here. It's probably because this test is very old that it wasn't there
| await page.waitForTimeout(1_000) // wait for export options to be available | ||
|
|
||
| // Select the first format option | ||
| const gltfOption = page.getByText('glTF') |
There was a problem hiding this comment.
nit: This could be cmdBar.selectOption({ name: 'glTF' }) I think
| const submitButton = page.getByText('Confirm Export') | ||
| await expect(submitButton).toBeVisible() | ||
|
|
||
| await page.waitForTimeout(500) | ||
| await page.keyboard.press('Enter') |
There was a problem hiding this comment.
I think I'd prefer await cmdBar.progressCmdBar() here for all these
Irev-Dev
left a comment
Irev-Dev
left a comment
There was a problem hiding this comment.
Had a few comments, I could have made more, but they'de all be nits where this old test doesn't use some of our newer fixture methods.
| await page.waitForTimeout(1_000) // wait for export options to be available | ||
|
|
||
| // Select the first format option | ||
| const gltfOption = page.getByText('glTF') | ||
| const exportFileName = `main.gltf` // source file is named `main.kcl` |
There was a problem hiding this comment.
Wouldn't await expect(gltfOption).toBeVisible() be better than an arbitary wait?
| await page.waitForTimeout(1_000) // wait for export options to be available | |
| // Select the first format option | |
| const gltfOption = page.getByText('glTF') | |
| const exportFileName = `main.gltf` // source file is named `main.kcl` | |
| // Select the first format option | |
| const gltfOption = page.getByText('glTF') | |
| await expect(gltfOption).toBeVisible() | |
| const exportFileName = `main.gltf` // source file is named `main.kcl` |
There was a problem hiding this comment.
Actually on second thought we have fixture tooling for the cmdbar.
example use
await cmdBar.expectState({
stage: 'arguments',
commandName: 'Import file from URL',
currentArgKey: 'method',
currentArgValue: '',
headerArguments: {
Method: '',
Name: 'test',
Code: '1 line',
},
highlightedHeaderArg: 'method',
})This second comment is more of a nit, understand you're just trying to make it more reliable, not refactor the code to match our newer patterns.
| await page.keyboard.press('Enter') | ||
|
|
||
| // Click the checkbox | ||
| const submitButton = page.getByText('Confirm Export') |
There was a problem hiding this comment.
Again nit, the same tool can do an assertion for the review stage of the cmdbar
await cmdBar.expectState({
stage: 'review',
commandName: 'Import file from URL',
headerArguments: {
Method: 'Existing project',
ProjectName: 'testProjectDir',
Name: 'test',
Code: '1 line',
},
})
|
Pierre and I were on the same wave length. |
* origin/main: (26 commits) attempt to import win-ca on windows (#6136) Upgrade e2e-tests windows runner from 4 cores to 8 (#6166) Follow-up fixes after bearing sample rename (#6164) Add test for #5799: "Only showing axis planes when there are no errors" (#6007) Wait for export button to make test more reliable (#6143) sketching on a mirror2d thats been extruded fixed! (#6149) Bump vite from 5.4.16 to 5.4.17 in /packages/codemirror-lang-kcl in the security group (#6150) Bump vite from 5.4.16 to 5.4.17 in the security group (#6151) Update all KCL-Samples to be more ME friendly (#6132) Shorten feedback cycle for legitimate failures (#6146) Remove the camera projection toggle from the UI (#6077) Use all available CPUs to run tests on CI (#6138) [fix] Get rid of risky useEffect in restart onboarding flow (#6133) Feature: Traditional menu actions in desktop application part II (#6030) [Bug] fix some UI friction from imports (#6139) Use scene fixture to make test more reliable on macOS (#6140) Fix: function composition during playwright setup created a massive page.reload loop (#6137) Alternative way to make appMachine spawned children type safe (#5890) [BUG] mutate ast to keep comments for pipe split ast-mod (#6128) Rename the app to Zoo Design Studio (#5974) ...
3 participants
The main source of flakiness in the
export works on the first trytest seems to be the export button in the panel not always being available right after the scene execution finishes. Adding a short delay ensures that clicking the button actually opens up the export menu.