Skip to content

Wait for export button to make test more reliable #6143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jacebrowning merged 1 commit into from
Apr 4, 2025
Merged

Wait for export button to make test more reliable #6143

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 42 additions & 57 deletions e2e/playwright/desktop-export.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,60 +32,52 @@ test(
})
await page.setBodyDimensions({ width: 1200, height: 500 })

page.on('console', console.log)

await test.step('on open of project', async () => {
await expect(page.getByText(`bracket`)).toBeVisible()

// open the project
await page.getByText(`bracket`).click()
// Open the project
const projectName = page.getByText(`bracket`)
await expect(projectName).toBeVisible()
await projectName.click()
await scene.waitForExecutionDone()
await page.waitForTimeout(1_000) // wait for panel buttons to be available

// expect zero errors in guter
// Expect zero errors in gutter
await expect(page.locator('.cm-lint-marker-error')).not.toBeVisible()

// export the model
// Click the export button
const exportButton = page.getByTestId('export-pane-button')
await expect(exportButton).toBeVisible()

await scene.waitForExecutionDone()

const gltfOption = page.getByText('glTF')
const submitButton = page.getByText('Confirm Export')
const exportingToastMessage = page.getByText(`Exporting...`)
const errorToastMessage = page.getByText(`Error while exporting`)
const engineErrorToastMessage = page.getByText(`Nothing to export`)
const alreadyExportingToastMessage = page.getByText(`Already exporting`)
// The open file's name is `main.kcl`, so the export file name should be `main.gltf`
const exportFileName = `main.gltf`

// Click the export button
await exportButton.click()
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`
Comment on lines +50 to +54
Copy link
Contributor

@Irev-Dev Irev-Dev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't await expect(gltfOption).toBeVisible() be better than an arbitary wait?

Suggested change
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`

Copy link
Contributor

@Irev-Dev Irev-Dev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 expect(gltfOption).toBeVisible()
await expect(page.getByText('STL')).toBeVisible()

await page.keyboard.press('Enter')

// Click the checkbox
const submitButton = page.getByText('Confirm Export')
Copy link
Contributor

@Irev-Dev Irev-Dev Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
        },
      })

await expect(submitButton).toBeVisible()

await page.waitForTimeout(500)

await page.keyboard.press('Enter')

// Find the toast.
// Look out for the toast message
const exportingToastMessage = page.getByText(`Exporting...`)
const alreadyExportingToastMessage = page.getByText(`Already exporting`)
await expect(exportingToastMessage).toBeVisible()
await expect(alreadyExportingToastMessage).not.toBeVisible()

// Expect it to succeed.
// Expect it to succeed
const errorToastMessage = page.getByText(`Error while exporting`)
const engineErrorToastMessage = page.getByText(`Nothing to export`)
await expect(errorToastMessage).not.toBeVisible()
await expect(engineErrorToastMessage).not.toBeVisible()

const successToastMessage = page.getByText(`Exported successfully`)
await expect(successToastMessage).toBeVisible()
await expect(exportingToastMessage).not.toBeVisible()

// Check for the exported file
const firstFileFullPath = path.resolve(
getPlaywrightDownloadDir(tronApp.projectDirName),
exportFileName
Expand All @@ -112,60 +104,53 @@ test(
const u = await getUtils(page)
await u.openFilePanel()

// Click on the other file
const otherKclButton = page.getByRole('button', { name: 'other.kcl' })

// Click the file
await otherKclButton.click()

// Close the file pane
await u.closeFilePanel()
await scene.waitForExecutionDone()
await page.waitForTimeout(1_000) // wait for panel buttons to be available

// FIXME: await scene.waitForExecutionDone() does not work. The modeling indicator stays in -receive-reliable and not execution done
await page.waitForTimeout(10000)

// expect zero errors in guter
// Expect zero errors in gutter
await expect(page.locator('.cm-lint-marker-error')).not.toBeVisible()

// export the model
// Click the export button
const exportButton = page.getByTestId('export-pane-button')
await expect(exportButton).toBeVisible()

const gltfOption = page.getByText('glTF')
const submitButton = page.getByText('Confirm Export')
const exportingToastMessage = page.getByText(`Exporting...`)
const errorToastMessage = page.getByText(`Error while exporting`)
const engineErrorToastMessage = page.getByText(`Nothing to export`)
const alreadyExportingToastMessage = page.getByText(`Already exporting`)
// The open file's name is `other.kcl`, so the export file name should be `other.gltf`
const exportFileName = `other.gltf`

// Click the export button
await exportButton.click()
await page.waitForTimeout(1_000) // wait for export options to be available
Copy link
Contributor

@pierremtb pierremtb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

options: [
{ name: 'glTF', isCurrent: true, value: 'gltf' },
{ name: 'OBJ', isCurrent: false, value: 'obj' },
{ name: 'STL', isCurrent: false, value: 'stl' },
{ name: 'STEP', isCurrent: false, value: 'step' },
{ name: 'PLY', isCurrent: false, value: 'ply' },
],

Copy link
Contributor

@pierremtb pierremtb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Select the first format option
const gltfOption = page.getByText('glTF')
Copy link
Contributor

@pierremtb pierremtb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This could be cmdBar.selectOption({ name: 'glTF' }) I think

const exportFileName = `other.gltf` // source file is named `other.kcl`
await expect(gltfOption).toBeVisible()
await expect(page.getByText('STL')).toBeVisible()

await page.keyboard.press('Enter')

// Click the checkbox
const submitButton = page.getByText('Confirm Export')
await expect(submitButton).toBeVisible()

await page.waitForTimeout(500)
await page.keyboard.press('Enter')
Comment on lines +132 to 135
Copy link
Contributor

@pierremtb pierremtb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer await cmdBar.progressCmdBar() here for all these


// Find the toast.
// Look out for the toast message
const exportingToastMessage = page.getByText(`Exporting...`)
const alreadyExportingToastMessage = page.getByText(`Already exporting`)
await expect(exportingToastMessage).toBeVisible()
await expect(alreadyExportingToastMessage).not.toBeVisible()

// Expect it to succeed
const errorToastMessage = page.getByText(`Error while exporting`)
const engineErrorToastMessage = page.getByText(`Nothing to export`)
await expect(errorToastMessage).not.toBeVisible()
await expect(engineErrorToastMessage).not.toBeVisible()

const successToastMessage = page.getByText(`Exported successfully`)
await test.step('Check the success toast message shows and nothing else', async () =>
Promise.all([
expect(alreadyExportingToastMessage).not.toBeVisible(),
expect(errorToastMessage).not.toBeVisible(),
expect(engineErrorToastMessage).not.toBeVisible(),
expect(successToastMessage).toBeVisible(),
expect(exportingToastMessage).not.toBeVisible(),
]))
await expect(successToastMessage).toBeVisible()
await expect(exportingToastMessage).not.toBeVisible()

// Check for the exported file=
const secondFileFullPath = path.resolve(
getPlaywrightDownloadDir(tronApp.projectDirName),
exportFileName
Expand Down
Loading