Fix test regression in asyncHelpers.js

[jedel1043]

Some recent changes to harness/asyncHelpers.js regressed some harness tests; test/harness/asyncHelpers-throwsAsync-func-throws-sync.js and test/harness/asyncHelpers-throwsAsync-no-arg.js to be more specific. The cause of this was fa3d024 and feb400c, which made some changes that broke those two tests.

This PR fixes this by slightly modifying asyncHelpers.js so that it passes the two tests, and also adds a new test to ensure #4186 is still fixed even with the new changes.

[gibson042]

I don't think I agree with this change... the provided func has correctly returned a thenable res, and I don't think we care about the value returned from res.then(onFulfilled, onRejected)—just that the appropriate callback will actually be invoked, and that is adequately covered by absorbing it with Promise.resolve.

However, I do see value in continuing to assert that res.then does not itself synchronously throw, which we can do by manually constructing the promise that follows res rather than using Promise.resolve.

Suggested change

	resolve(Promise.resolve(res).then(
	thenable = res.then(
	var onResFulfilled, onResRejected;
	var resSettlementP = new Promise(function (onFulfilled, onRejected) {
	onResFulfilled = onFulfilled;
	onResRejected = onRejected;
	});
	res.then(onResFulfilled, onResRejected);
	resolve(resSettlementP.then(

[jedel1043]

Yeah, that's a much cleaner approach.

[gibson042]

👍

[gibson042]

This would be better named something like secondaryThenable, but I'm pretty sure assert.throwsAsync is better without having it at all.

[gibson042]

I find this message is misleading; the result (res) is conforming as long as res.then(onFulfilled, onRejected) eventually invokes onFulfilled to indicate func() success or onRejected to indicate func() failure, even if res.then does not itself return a thenable—we only care about the callbacks, not about further chainability. But if we did care, I would suggest instead " but result thenable's "then" method did not return a thenable".

[gibson042]

The problem with then: function () {} isn't that it fails to return a Promise, it's that it fails to invoke either its first or second argument, leaving this p permanently unsettled—and I don't think we can test for that. Put another way, then: function (_onFulfilled, _onRejected) { return Promise.resolve(); } is just as bad for actual use of asyncTest, but would not trip any assertions due to the very nature of asynchronous code.

[jedel1043]

Yeah, that sounds fair. However, I was thinking of ways to test this and maybe it's possible to test with something like:

assert.throwsAsync(TypeError, func).then(
  function() { $DONE("the promise should not be resolved") },
  function() { $DONE("the promise should not be resolved") },
);
$DONE();

Though, I'm still unsure if we can assume that implementations will keep running jobs even if the print function already printed Test262:AsyncTestComplete, but that's maybe something other tests already assume?

[gibson042]

Though, I'm still unsure if we can assume that implementations will keep running jobs even if the print function already printed Test262:AsyncTestComplete, but that's maybe something other tests already assume?

Presumably. If we're going to cover this, I guess it would be modeled on something like test/harness/asyncHelpers-asyncTest-returns-undefined.js and just overwrite $DONE plus document a timeliness assumption:

var realDone = $DONE;
var doneArgs = [];
globalThis.$DONE = function (arg) {
  doneArgs.push(arg);
};

function delay() {
  var later = Promise.resolve();
  for (var i = 0; i < 100; i++) {
    later = later.then();
  }
  return later;
}

(async function () {
  // Spy on the promise returned by an invocation of assert.throwsAsync with a
  // function that returns a thenable which never invokes provided callbacks.
  var blackHoleThenable = { then: function() {} };
  var p = assert.throwsAsync(TypeError, function() { return blackHoleThenable; });
  p.then($DONE, $DONE);
})()
  // Give it a long time to try.
  .then(delay, delay)
  .then(function () {
    assert.compareArray(doneArgs, [], "$DONE must not have be called");
  })
  .then(realDone, realDone);

[jedel1043]

Sounds good! I'll change the test