From 3faec650b1cc90f6caa8df4312d69897b94eac94 Mon Sep 17 00:00:00 2001 From: Jess Frazelle Date: Wed, 14 Aug 2024 08:49:00 -0700 Subject: [PATCH] ensure we never execute over ourselves (#3419) * ensure we never execute over ourselves Signed-off-by: Jess Frazelle * fixups Signed-off-by: Jess Frazelle * cleanup Signed-off-by: Jess Frazelle * updates Signed-off-by: Jess Frazelle * updates Signed-off-by: Jess Frazelle * weird logs Signed-off-by: Jess Frazelle * fix flake Signed-off-by: Jess Frazelle * make faster Signed-off-by: Jess Frazelle --------- Signed-off-by: Jess Frazelle --- e2e/playwright/regression-tests.spec.ts | 79 ++++++++-------------- src/components/ModelingMachineProvider.tsx | 2 - src/lang/KclSingleton.ts | 68 +++++++++++++------ src/lang/std/artifactGraph.test.ts | 2 +- 4 files changed, 77 insertions(+), 74 deletions(-) diff --git a/e2e/playwright/regression-tests.spec.ts b/e2e/playwright/regression-tests.spec.ts index ed6fd2e1f..3d7f9311a 100644 --- a/e2e/playwright/regression-tests.spec.ts +++ b/e2e/playwright/regression-tests.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '@playwright/test' +import { test, expect, Page } from '@playwright/test' import { getUtils, setup, tearDown } from './test-utils' import { TEST_CODE_TRIGGER_ENGINE_EXPORT_ERROR } from './storageStates' @@ -346,55 +346,24 @@ const sketch001 = startSketchAt([-0, -0]) // expect zero errors in guter await expect(page.locator('.cm-lint-marker-error')).not.toBeVisible() - // export the model - const exportButton = page.getByTestId('export-pane-button') - await expect(exportButton).toBeVisible() - - // Click the export button - exportButton.click() - - // Click the stl. - const stlOption = page.getByText('glTF') - await expect(stlOption).toBeVisible() - - await page.keyboard.press('Enter') - - // Click the checkbox - const submitButton = page.getByText('Confirm Export') - await expect(submitButton).toBeVisible() - - await page.keyboard.press('Enter') - - // Find the toast. - // Look out for the toast message - const exportingToastMessage = page.getByText(`Exporting...`) - await expect(exportingToastMessage).toBeVisible() - const errorToastMessage = page.getByText(`Error while exporting`) - + const exportingToastMessage = page.getByText(`Exporting...`) const engineErrorToastMessage = page.getByText(`Nothing to export`) - const alreadyExportingToastMessage = page.getByText(`Already exporting`) - // Try exporting again. - // Click the export button - exportButton.click() + await clickExportButton(page) - // Click the stl. - await expect(stlOption).toBeVisible() + await expect(exportingToastMessage).toBeVisible() - await page.keyboard.press('Enter') - - // Click the checkbox - await expect(submitButton).toBeVisible() - - await page.keyboard.press('Enter') + await clickExportButton(page) // Find the toast. // Look out for the toast message await expect(exportingToastMessage).toBeVisible() await expect(alreadyExportingToastMessage).toBeVisible() + await page.waitForTimeout(1000) + // Expect it to succeed. await expect(exportingToastMessage).not.toBeVisible() await expect(errorToastMessage).not.toBeVisible() @@ -406,18 +375,7 @@ const sketch001 = startSketchAt([-0, -0]) await expect(alreadyExportingToastMessage).not.toBeVisible() // Try exporting again. - // Click the export button - exportButton.click() - - // Click the stl. - await expect(stlOption).toBeVisible() - - await page.keyboard.press('Enter') - - // Click the checkbox - await expect(submitButton).toBeVisible() - - await page.keyboard.press('Enter') + await clickExportButton(page) // Find the toast. // Look out for the toast message @@ -432,3 +390,24 @@ const sketch001 = startSketchAt([-0, -0]) await expect(successToastMessage).toBeVisible() }) }) + +async function clickExportButton(page: Page) { + // export the model + const exportButton = page.getByTestId('export-pane-button') + await expect(exportButton).toBeVisible() + + // Click the export button + exportButton.click() + + // Click the stl. + const gltfOption = page.getByText('glTF') + await expect(gltfOption).toBeVisible() + + await page.keyboard.press('Enter') + + // Click the checkbox + const submitButton = page.getByText('Confirm Export') + await expect(submitButton).toBeVisible() + + await page.keyboard.press('Enter') +} diff --git a/src/components/ModelingMachineProvider.tsx b/src/components/ModelingMachineProvider.tsx index 45e895247..373abaed2 100644 --- a/src/components/ModelingMachineProvider.tsx +++ b/src/components/ModelingMachineProvider.tsx @@ -149,8 +149,6 @@ export const ModelingMachineProvider = ({ }, 'sketch exit execute': ({ store }) => { ;(async () => { - // blocks entering a sketch until after exit sketch code has run - kclManager.isExecuting = true sceneInfra.camControls.syncDirection = 'clientToEngine' await sceneInfra.camControls.snapToPerspectiveBeforeHandingBackControlToEngine() diff --git a/src/lang/KclSingleton.ts b/src/lang/KclSingleton.ts index 10c80ccbc..4a7604b3a 100644 --- a/src/lang/KclSingleton.ts +++ b/src/lang/KclSingleton.ts @@ -19,6 +19,16 @@ import { getNodeFromPath } from './queryAst' import { codeManager, editorManager, sceneInfra } from 'lib/singletons' import { Diagnostic } from '@codemirror/lint' +interface ExecuteArgs { + ast?: Program + zoomToFit?: boolean + executionId?: number + zoomOnRangeAndType?: { + range: SourceRange + type: string + } +} + export class KclManager { private _ast: Program = { body: [], @@ -36,6 +46,7 @@ export class KclManager { private _lints: Diagnostic[] = [] private _kclErrors: KCLError[] = [] private _isExecuting = false + private _executeIsStale: ExecuteArgs | null = null private _wasmInitFailed = true engineCommandManager: EngineCommandManager @@ -113,9 +124,25 @@ export class KclManager { } set isExecuting(isExecuting) { this._isExecuting = isExecuting + // If we have finished executing, but the execute is stale, we should + // execute again. + if (!isExecuting && this.executeIsStale) { + const args = this.executeIsStale + this.executeIsStale = null + this.executeAst(args) + } else { + } this._isExecutingCallback(isExecuting) } + get executeIsStale() { + return this._executeIsStale + } + + set executeIsStale(executeIsStale) { + this._executeIsStale = executeIsStale + } + get wasmInitFailed() { return this._wasmInitFailed } @@ -202,16 +229,16 @@ export class KclManager { // This NEVER updates the code, if you want to update the code DO NOT add to // this function, too many other things that don't want it exist. // just call to codeManager from wherever you want in other files. - async executeAst( - ast: Program = this._ast, - zoomToFit?: boolean, - executionId?: number, - zoomOnRangeAndType?: { - range: SourceRange - type: string + async executeAst(args: ExecuteArgs = {}): Promise { + if (this.isExecuting) { + this.executeIsStale = args + // Exit early if we are already executing. + return } - ): Promise { - const currentExecutionId = executionId || Date.now() + + const ast = args.ast || this.ast + + const currentExecutionId = args.executionId || Date.now() this._cancelTokens.set(currentExecutionId, false) this.isExecuting = true @@ -229,12 +256,12 @@ export class KclManager { defaultSelectionFilter(programMemory, this.engineCommandManager) await this.engineCommandManager.waitForAllCommands() - if (zoomToFit) { + if (args.zoomToFit) { let zoomObjectId: string | undefined = '' - if (zoomOnRangeAndType) { + if (args.zoomOnRangeAndType) { zoomObjectId = this.engineCommandManager?.mapRangeToObjectId( - zoomOnRangeAndType.range, - zoomOnRangeAndType.type + args.zoomOnRangeAndType.range, + args.zoomOnRangeAndType.type ) } @@ -259,6 +286,7 @@ export class KclManager { } this.isExecuting = false + // Check the cancellation token for this execution before applying side effects if (this._cancelTokens.get(currentExecutionId)) { this._cancelTokens.delete(currentExecutionId) @@ -351,8 +379,7 @@ export class KclManager { return } this.ast = { ...ast } - this.isExecuting = true // executeAst sets this to false again - return this.executeAst(ast, zoomToFit) + return this.executeAst({ zoomToFit }) } format() { const originalCode = codeManager.code @@ -430,12 +457,11 @@ export class KclManager { codeManager.updateCodeEditor(newCode) // Write the file to disk. await codeManager.writeToFile() - await this.executeAst( - astWithUpdatedSource, - optionalParams?.zoomToFit, - undefined, - optionalParams?.zoomOnRangeAndType - ) + await this.executeAst({ + ast: astWithUpdatedSource, + zoomToFit: optionalParams?.zoomToFit, + zoomOnRangeAndType: optionalParams?.zoomOnRangeAndType, + }) } else { // When we don't re-execute, we still want to update the program // memory with the new ast. So we will hit the mock executor diff --git a/src/lang/std/artifactGraph.test.ts b/src/lang/std/artifactGraph.test.ts index 0a900da97..7d6942b5a 100644 --- a/src/lang/std/artifactGraph.test.ts +++ b/src/lang/std/artifactGraph.test.ts @@ -136,7 +136,7 @@ beforeAll(async () => { console.error(ast) return Promise.reject(ast) } - await kclManager.executeAst(ast) + await kclManager.executeAst({ ast }) cacheToWriteToFileTemp[codeKey] = { orderedCommands: engineCommandManager.orderedCommands,