diff --git a/cypress/e2e/spec-wc-skulpt.cy.js b/cypress/e2e/spec-wc-skulpt.cy.js index d2fe74f18..d3114475a 100644 --- a/cypress/e2e/spec-wc-skulpt.cy.js +++ b/cypress/e2e/spec-wc-skulpt.cy.js @@ -60,6 +60,29 @@ describe("Running the code with skulpt", () => { getP5Canvas().should("exist"); }); + it("shows an error and stops the sketch when a p5 preload raises", () => { + // preload/setup run before any canvas exists, so VisualOutputPane can't + // stop the sketch. The shim must stop it, otherwise p5 treats the callback + // as successful and carries on into setup/draw - here draw() would print, + // which is asserted never to happen. + runCode( + "from p5 import *\n\ndef preload():\n\traise ValueError('boom in preload')\ndef setup():\n\tsize(400, 400)\ndef draw():\n\tprint('draw ran')\nrun(frame_rate=2)", + ); + + getErrorMessage().should("contain.text", "ValueError: boom in preload"); + getSkulptRunner().should("not.contain", "draw ran"); + }); + + it("shows an error message when a p5 sketch raises in draw", () => { + // draw() runs every frame via requestAnimationFrame; the shim routes its + // errors to the editor rather than window.onerror. + runCode( + "from p5 import *\n\ndef setup():\n\tsize(400, 400)\ndef draw():\n\traise ValueError('boom in draw')\nrun(frame_rate=2)", + ); + + getErrorMessage().should("contain.text", "ValueError: boom in draw"); + }); + it("runs a simple py5 program", () => { runCode( "import py5\ndef setup():\n\tpy5.size(400, 400)\ndef draw():\n\tpy5.background(255)\npy5.run_sketch()", diff --git a/cypress/helpers/editor.js b/cypress/helpers/editor.js index bc2c570a3..79dcce329 100644 --- a/cypress/helpers/editor.js +++ b/cypress/helpers/editor.js @@ -93,7 +93,11 @@ const dragHandle = (getHandle, { deltaX = 0, deltaY = 0 }) => { const clientX = left + width / 2; const clientY = top + height / 2; - cy.wrap($handle).trigger("mousedown", { button: 0, clientX, clientY }); + // Re-query the handle instead of reusing $handle. If the app re-renders + // while trigger() is waiting for the element to become actionable, a + // wrapped element is detached for good and trigger() burns its full + // timeout; a query chain retries against the current element. + getHandle().trigger("mousedown", { button: 0, clientX, clientY }); getHandle() .parent("div") @@ -125,6 +129,14 @@ export const openFilePanel = () => export const loadPythonStarterProject = () => { cy.findByText("blank-python-starter").click(); + + // web-component.html fetches the project JSON, then throws away the current + // and prepends a brand new one. The component being replaced + // already has a visible Run button, so waiting on that alone can resolve + // against the doomed component and leave the test holding elements that + // detach part-way through. Only the replacement publishes an identifier. + cy.get("#project-identifier").should("have.text", "blank-python-starter"); + getEditorShadow().findByRole("button", { name: /run/i }).should("be.visible"); }; diff --git a/public/shims/processing/p5/p5-shim.js b/public/shims/processing/p5/p5-shim.js index 510c1f9b6..bd1dba77d 100644 --- a/public/shims/processing/p5/p5-shim.js +++ b/public/shims/processing/p5/p5-shim.js @@ -1510,47 +1510,76 @@ const $builtinmodule = function (name) { Sk.builtin.str("py5_imported"), ].includes(mod.__name__); + // p5 runs preload/setup/draw asynchronously, so an uncaught exception + // there escapes to window.onerror. Catching it isn't enough on its own: + // p5 would treat the callback as successful and carry on into the draw + // loop, and VisualOutputPane only stops the sketch once a canvas exists - + // so a failure before then would keep looping. Surface the error and stop + // the loop here, and skip the remaining callbacks. + let sketchStopped = false; + const stopOnError = (e) => { + sketchStopped = true; + mod.pInst?.noLoop(); + Sk.uncaughtException(e); + }; + sketch.preload = function () { if (Sk.globals["preload"] && !isPy5Version) { - Sk.misceval.callsimArray(Sk.globals["preload"]); + try { + Sk.misceval.callsimArray(Sk.globals["preload"]); + } catch (e) { + stopOnError(e); + } } }; sketch.setup = function () { - if (Sk.globals["settings"] && isPy5Version) { - Sk.misceval.callsimArray(Sk.globals["settings"]); + if (sketchStopped) { + return; } - if (Sk.globals["setup"]) { - Sk.misceval.callsimArray(Sk.globals["setup"]); - - for (const cb of Object.keys(callBacks)) { - if (Sk.globals[cb]) { - sketch[callBacks[cb]] = new Function( - "try {Sk.misceval.callsimArray(Sk.globals['" + - cb + - "']);} catch(e) {Sk.uncaughtException(e);}", - ); + try { + if (Sk.globals["settings"] && isPy5Version) { + Sk.misceval.callsimArray(Sk.globals["settings"]); + } + if (Sk.globals["setup"]) { + Sk.misceval.callsimArray(Sk.globals["setup"]); + + for (const cb of Object.keys(callBacks)) { + if (Sk.globals[cb]) { + sketch[callBacks[cb]] = new Function( + "try {Sk.misceval.callsimArray(Sk.globals['" + + cb + + "']);} catch(e) {Sk.uncaughtException(e);}", + ); + } } } + } catch (e) { + stopOnError(e); } }; mod.pInst.frameRate(frame_rate.v); sketch.draw = function () { - mod.pInst.scale(scaleFactor, scaleFactor); - if (mod.__name__ === Sk.builtin.str("py5")) { - mod.frame_count = new Sk.builtin.int_(sketch.frameCount); - } else { - Sk.builtins.frame_count = new Sk.builtin.int_(sketch.frameCount); + if (sketchStopped) { + return; } + // Wrap the whole body (not just the user draw() call) - draw runs every + // frame and any exception would otherwise escape to window.onerror. + try { + mod.pInst.scale(scaleFactor, scaleFactor); + if (mod.__name__ === Sk.builtin.str("py5")) { + mod.frame_count = new Sk.builtin.int_(sketch.frameCount); + } else { + Sk.builtins.frame_count = new Sk.builtin.int_(sketch.frameCount); + } - if (Sk.globals["draw"]) { - try { + if (Sk.globals["draw"]) { Sk.misceval.callsimArray(Sk.globals["draw"]); - } catch (e) { - Sk.uncaughtException(e); } + } catch (e) { + stopOnError(e); } }; diff --git a/public/shims/processing/py5/py5-shim.js b/public/shims/processing/py5/py5-shim.js index 640eaa2d2..73ba65925 100644 --- a/public/shims/processing/py5/py5-shim.js +++ b/public/shims/processing/py5/py5-shim.js @@ -1473,33 +1473,55 @@ const $builtinmodule = function (name) { mod.pInst = sketch; + // p5 runs settings/setup/draw asynchronously, so an uncaught exception + // there escapes to window.onerror. Catching it isn't enough on its own: + // p5 would treat the callback as successful and carry on into the draw + // loop, and VisualOutputPane only stops the sketch once a canvas exists - + // so a failure before then would keep looping. Surface the error and stop + // the loop here, and skip the remaining callbacks. + let sketchStopped = false; + const stopOnError = (e) => { + sketchStopped = true; + mod.pInst?.noLoop(); + Sk.uncaughtException(e); + }; + sketch.setup = function () { - if (Sk.globals["settings"]) { - Sk.misceval.callsimArray(Sk.globals["settings"]); - } - if (Sk.globals["setup"]) { - Sk.misceval.callsimArray(Sk.globals["setup"]); - - for (const cb of Object.keys(callBacks)) { - if (Sk.globals[cb]) { - sketch[callBacks[cb]] = new Function( - "try {Sk.misceval.callsimArray(Sk.globals['" + - cb + - "']);} catch(e) {Sk.uncaughtException(e);}", - ); + try { + if (Sk.globals["settings"]) { + Sk.misceval.callsimArray(Sk.globals["settings"]); + } + if (Sk.globals["setup"]) { + Sk.misceval.callsimArray(Sk.globals["setup"]); + + for (const cb of Object.keys(callBacks)) { + if (Sk.globals[cb]) { + sketch[callBacks[cb]] = new Function( + "try {Sk.misceval.callsimArray(Sk.globals['" + + cb + + "']);} catch(e) {Sk.uncaughtException(e);}", + ); + } } } + } catch (e) { + stopOnError(e); } }; sketch.draw = function () { - mod.frame_count = new Sk.builtin.int_(sketch.frameCount); - if (Sk.globals["draw"]) { - try { + if (sketchStopped) { + return; + } + // Wrap the whole body (not just the user draw() call) - draw runs every + // frame and any exception would otherwise escape to window.onerror. + try { + mod.frame_count = new Sk.builtin.int_(sketch.frameCount); + if (Sk.globals["draw"]) { Sk.misceval.callsimArray(Sk.globals["draw"]); - } catch (e) { - Sk.uncaughtException(e); } + } catch (e) { + stopOnError(e); } }; diff --git a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx index 647232b7c..e300ebe27 100644 --- a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx +++ b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx @@ -412,19 +412,25 @@ const SkulptRunner = ({ message: errorMessage, }; } else { - const errorDescription = (err.tp$str && err.tp$str().v) + // err is normally a Skulpt exception (tp$str/traceback), but native + // code called from Python (e.g. the p5/py5 shims) can throw plain JS + // errors instead, which don't have that shape - fall back gracefully + // rather than letting this handler itself throw. + const rawDescription = + typeof err.tp$str === "function" ? err.tp$str().v : undefined; + const errorDescription = (rawDescription || err.message || "") .replace(/\[(.*?)\]/, "") .replace(/\.$/, ""); - const errorType = err.tp$name || err.constructor.name; - let lineNumber = err.traceback[0].lineno; - let fileName = err.traceback[0].filename; + const errorType = err.tp$name || err.constructor?.name; + let lineNumber = err.traceback?.[0]?.lineno; + let fileName = err.traceback?.[0]?.filename; // If this is an error in the sense_hat.py shim, use the next entry in // the traceback as this will be the line in the shim which we don't want // to show to users, so that the error message will instead point to the // line in the user's code which caused the error. if ( - err.traceback.length > 1 && + err.traceback?.length > 1 && fileName === "./sense_hat.py" && ["RuntimeError", "ValueError"].includes(errorType) ) { @@ -432,7 +438,7 @@ const SkulptRunner = ({ fileName = err.traceback[1].filename; } - fileName = fileName.replace(/^\.\//, ""); + fileName = fileName ? fileName.replace(/^\.\//, "") : fileName; if (errorType === "ImportError" && window.crossOriginIsolated) { const articleLink = `https://help.editor.raspberrypi.org/hc/en-us/articles/30841379339924-What-Python-libraries-are-available-in-the-Code-Editor`; @@ -447,7 +453,9 @@ const SkulptRunner = ({ const { createError } = ApiCallHandler({ reactAppApiEndpoint }); - errorMessage = `${errorType}: ${errorDescription} on line ${lineNumber} of ${fileName}${ + const location = + lineNumber && fileName ? ` on line ${lineNumber} of ${fileName}` : ""; + errorMessage = `${errorType}: ${errorDescription}${location}${ explanation ? `. ${explanation}` : "" }`; createError(projectIdentifier, userId, { diff --git a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.jsx b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.jsx index cd60876eb..b2e2e659e 100644 --- a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.jsx +++ b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.jsx @@ -396,6 +396,77 @@ describe("When an error originates in the sense_hat shim", () => { }); }); +describe("When native code called from Python throws a plain JS error (e.g. the p5/py5 shims), rather than a Skulpt exception", () => { + let store; + let asyncToPromiseSpy; + + beforeEach(() => { + asyncToPromiseSpy = jest + .spyOn(Sk.misceval, "asyncToPromise") + .mockReturnValue( + Promise.reject( + new TypeError("Cannot read properties of undefined (reading 'foo')"), + ), + ); + + const middlewares = []; + const mockStore = configureMockStore(middlewares); + const initialState = { + editor: { + project: { + components: [ + { + name: "main", + extension: "py", + content: "import py5", + }, + ], + image_list: [], + }, + codeRunTriggered: true, + }, + auth: { + user, + }, + }; + store = mockStore(initialState); + render( + + + , + ); + }); + + afterEach(() => { + asyncToPromiseSpy.mockRestore(); + }); + + test("falls back to the native error message instead of throwing", () => { + expect(store.getActions()).toEqual( + expect.arrayContaining([ + setError( + "TypeError: Cannot read properties of undefined (reading 'foo')", + ), + ]), + ); + }); + + test("sets errorDetails using the native error type and message", () => { + expect(store.getActions()).toEqual( + expect.arrayContaining([ + setErrorDetails({ + type: "TypeError", + line: undefined, + file: undefined, + description: "Cannot read properties of undefined (reading 'foo')", + message: + "TypeError: Cannot read properties of undefined (reading 'foo')", + }), + ]), + ); + }); +}); + describe("When an error has occurred", () => { let mockStore; let store;