Core: Error when a module callback has a promise as a return value

[raycohen]

Ref https://github.com/qunitjs/qunit/issues/1600.

For the QUnit version 3.0 release.

I'd have moved the tests for this into test/main/modules.js but I am not able to use async/await syntax nor promises in that file.

Recreation of #1616

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: raycohen / name: Ray Cohen (c2e35da317f2ff864198db3b2923ed1197243b37, 1af57d7bcf9da03728286e9141ec5baa95b91e58, 7e0dbe6b1c41ba04d6624498ab83ec3dfe48fd6e, b1430dea1e12f1c7c64700bede993a254fab700d, 640727c0aefa50e0d0fbe261485cf4ba34891213, e60323b3a1e72001167a3f781d770f0b10db2451, 14a6a1fc7131e2c5095c031221fb26152e1a9349, 782f22f1a587d2e5f9c20c9b90575658ce10c04c, 77265aba8fdde7c18b9728f8dff2ba1c7ac05e8c, 76ad95aaa2854665c996ad8769b0815dbaf1dec1, 527d2990f6eb27dfc66cbb0f5ee983e00dffc94c)

[Krinkle]

@raycohen I suggest adding it there with a thenable as return instead.

Like so:

QUnit.module('example', function () {
  QUnit.test('test', function (assert) {
    assert.true(true);
  });

  return { then: function () {} };
});

By the way, upon first reading the test, I was hoping you'd make it a failing test, e.g. assert.true(false) on the expectation that the test doesn't actually run since we throw right away. However... despite the seemingly "fast" failure through this uncaught error, the tests would still get defined and run regardless (with any async ones going to an unrelated module in all likelihood). This seems unavoidable without more changes, perhaps best outside the scope of this PR. But it did make me think if there's a way we could detect an AsyncFunction without calling it. It looks like there is?

• https://262.ecma-international.org/12.0/#sec-runtime-semantics-namedevaluation
• https://262.ecma-international.org/12.0/#sec-runtime-semantics-instantiateasyncfunctionexpression AsyncFunction.prototype

I haven't yet looked at what, if any, established idiom there is for this, but, if nothing else, one could probably use Object.prototype.toString(fn) === "[object AsyncFunction]" and skip the call in those cases to avoid starting an untracked asynchronous call stack. What do you think about that? The downside would be that we'd only do that for true/native async functions and not those that are compiled to (or written as) regular functions that merely return promises. Given this is error handling, maybe the inconsistency isn't too bad, for the benefit of less noise/other issues being produced.

[raycohen]

I added an additional { then: function () {} } fixture, renamed the other two fixtures to "error" instead of "warning", and added the early bail when the async function can be detected.

I'm not sure what's happening with the linter on the Linux ci builds saying Logger is unused.

[Krinkle]

I'm not sure what's happening with the linter on the Linux ci builds saying Logger is unused.

You may have to rebase this locally first. The main branch has seen a few commits since, leaving the call to Logger.warn() that your patch removes, as the only one remaining. Thus it is unused from the state of the repo that CI sees (which is, simulate a merge of the PR into latest main, then run the CI jobs).

[raycohen]

Given this is error handling, maybe the inconsistency isn't too bad, for the benefit of less noise/other issues being produced.

I agree the Async function deserves special treatment since we can detect it. I'm guessing it's the most common one the most users will try doing as well