feat: drop support for function tests and unawaited promises (#406)

* feat: drop support for function tests and unawaited promises

() => assert(...)

will simply be evaluated, the function will no longer
get called.

new Promise(...)

will not be awaited, but

await new Promise(...)

will be.

This is much closer to how normal javascript and normal tests behave.

* fix: ignore returned Promises in python tests

Author: ojeytonwilliams

packages/dom-evaluator/src/dom-test-evaluator.ts

@@ -129,31 +129,10 @@ export class DOMTestEvaluator implements TestEvaluator {
     this.#runTest = async function (rawTest: string): Promise<Fail | Pass> {
       this.#proxyConsole.on();
       try {
-        let test;
-        try {
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-          test = await eval(`${opts.hooks?.beforeEach ?? ""}
-${rawTest}`);
-        } catch (err) {
-          if (
-            err instanceof SyntaxError &&
-            err.message.includes(
-              "await is only valid in async functions and the top level bodies of modules",
-            )
-          ) {
-            const iifeTest = createAsyncIife(rawTest);
-            // There's no need to assign this to 'test', since it replaces that
-            // functionality.
-            await eval(iifeTest);
-          } else {
-            throw err;
-          }
-        }
+        const test = createAsyncIife(rawTest);
 
-        if (typeof test === "function") {
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-          await test();
-        }
+        await eval(`${opts.hooks?.beforeEach ?? ""}
+${test}`);
 
         return { pass: true, ...this.#proxyConsole.flush() };
       } catch (err) {

packages/python-evaluator/src/python-test-evaluator.ts

@@ -112,35 +112,36 @@ input = __fake_input
 
       try {
         eval(opts.hooks?.beforeEach ?? "");
-        // Eval test string to get the dummy input and actual test
-        const evaluatedTestString = await new Promise<unknown>(
-          (resolve, reject) => {
-            try {
-              const test: unknown = eval(testString);
-              resolve(test);
-            } catch (err) {
-              const isUsingTopLevelAwait =
-                err instanceof SyntaxError &&
-                err.message.includes(
-                  "await is only valid in async functions and the top level bodies of modules",
-                );
-
-              if (isUsingTopLevelAwait) {
-                const iifeTest = createAsyncIife(testString);
-
-                // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
-                eval(iifeTest).then(resolve).catch(reject);
-              } else {
-                // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors
-                reject(err);
-              }
-            }
-          },
-        );
-
-        // If the test string does not evaluate to an object, then we assume that
-        // it's a standard JS test and any assertions have already passed.
-        if (typeof evaluatedTestString !== "object") {
+        // Eval test string to get the test property
+
+        let evaluatedTestString;
+
+        try {
+          evaluatedTestString = eval(testString) as EvaluatedTeststring;
+        } catch (err) {
+          const isUsingTopLevelAwait =
+            err instanceof SyntaxError &&
+            err.message.includes(
+              "await is only valid in async functions and the top level bodies of modules",
+            );
+
+          if (isUsingTopLevelAwait) {
+            const iifeTest = createAsyncIife(testString);
+
+            await eval(iifeTest);
+          } else {
+            throw err;
+          }
+        }
+
+        // If the test string does not evaluate to an object, then we assume
+        // that it's a standard JS test and any assertions have already passed.
+        // Finally, if it is a Promise, that should have been awaited so we
+        // ignore it.
+        if (
+          typeof evaluatedTestString !== "object" ||
+          evaluatedTestString instanceof Promise
+        ) {
           // Execute afterEach hook if it exists
           if (opts.hooks?.afterEach) eval(opts.hooks.afterEach);
 
@@ -153,7 +154,7 @@ input = __fake_input
           );
         }
 
-        const { test } = evaluatedTestString as EvaluatedTeststring;
+        const { test } = evaluatedTestString;
 
         // Evaluates the learner's code so that any variables they define are
         // available to the test.

packages/tests/dom-test-evaluator.test.ts

@@ -83,21 +83,5 @@ describe("DOMTestEvaluator", () => {
 
       expect(result).toStrictEqual({ pass: true });
     });
-
-    it("should await tests that return promises", async () => {
-      const test = `new Promise((resolve) => setTimeout(resolve, 1))
-			  .then(() => { assert.equal(1,2)});`;
-
-      const result = await evaluator.runTest(test);
-      expect(result).toStrictEqual({
-        err: {
-          message: "expected 1 to equal 2",
-          expected: 2,
-          actual: 1,
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-          stack: expect.stringMatching("AssertionError: expected"),
-        },
-      });
-    });
   });
 });

packages/tests/integration-tests/index.test.ts

@@ -536,26 +536,24 @@ for(let i = 0; i < 3; i++) {
       expect(result).toMatchObject({ err: { actual: 1 } });
     });
 
-    // The old api wasn't consistent: only DOM tests supported async tests, but
-    // this needs to be maintained while it's deprecated.
-    it.runIf(type === "dom")(
-      "should support `async () => {}` style tests",
-      async () => {
-        const result = await page.evaluate(async (type) => {
-          const runner = await window.FCCTestRunner.createTestRunner({
-            type,
-          });
+    it("should not await tests that return promises", async () => {
+      const result = await page.evaluate(async (type) => {
+        const runner = await window.FCCTestRunner.createTestRunner({
+          type,
+        });
 
-          return runner.runTest(`
-          async () => {
-            const delay = (ms) => new Promise(resolve => setTimeout(resolve, ms));
-            await delay(10);
-            assert.equal(1, 2);
-          }`);
-        }, type);
-        expect(result).toMatchObject({ err: { actual: 1 } });
-      },
-    );
+        return runner.runTest(`
+new Promise((resolve) => {
+  setTimeout(() => {
+    console.log("This should not be awaited");
+    resolve();
+  }, 10);
+});
+`);
+      }, type);
+      // No logs should be returned, as the test should not be awaited
+      expect(result).toEqual({ pass: true });
+    });
 
     it("should fail the test if the afterEach hook throws an error", async () => {
       const result = await page.evaluate(async (type) => {
@@ -936,9 +934,7 @@ const getFive = () => 5;
           source,
           type: "dom",
         });
-        return runner.runTest(
-          "async () => await document.getElementById('audio').play()",
-        );
+        return runner.runTest("await document.getElementById('audio').play()");
       }, source);
 
       // If it were unable to play, it would throw "play() failed because the