Introducing "it.yield", a convenient way to test promise code

[lannka]

We're often facing tests like this:
Example A

verify(false);
resolver();  // this resolve a promise
verify(true); // this will fail because the promise chain is not executed yet.

Hack A: We are smart, so we figured we can:

verify(false);
resolver();  // this resolve a promise
// wait promise to resolve 
return widget.promise_.then(() => {
  verify(true);
});

Example B

verify(false);
clock.tick(100);  // this triggers a timer and then a promise chain
verify(true); // this will fail because the promise chain is not resolved yet.

Hack B:

verify(false);
clock.tick(100);  // this triggers a timer and then a promise chain

setTimeout(() => {
  verify(true); 
  done();
})

Hack B+: This can easily go out of control:

verify(false);
clock.tick(99); 

setTimeout(() => {
  verify(false); 
  clock.tick(1);

  setTimeout(() => {
    verify(true);
    done();
  });
})

Introducing it.yield, no more callback hell! :

verify(false);
clock.tick(99);
yield;
verify(false);
clock.tick(1);
yield;
verify(true);

/cc @cramforce @dvoytenko

(One caveat: Safari 9 does not support Generator)

[cramforce]

This is genius, but I don't fully understand how it works. Or this is a bit awkward.

Wouldn't it be better to always yield a promise instead of relying on the magic setInterval shortcircuiting in the trivial case?

If you return a promise from incrementLoadingAds that would work for this test.

[lannka]

When code runs to ayield statement in a Generator, it stops and yield to other tasks/micro tasks in the event loop to run. It will resume once the Generator.next() is called.

Calling Generator.next() in a setInterval makes sure the remaining work always get queued to the end of event loop.

[dreamofabear]

Super cool, just learned about generators and yield. 😄

Could await and Promises could achieve the same goal (but perhaps read more naturally)? E.g. in the test below:

let loadAds = incrementLoadingAds(win); // returns Promise
expect(is3pThrottled(win)).to.be.true;
await loadAds; // replaces invocation of resolver and `yield`
expect(is3pThrottled(win)).to.be.false;

[lannka]

depending on the use cases. here we do have a handle of the promise, but in many real scenario, we don't.

also, await is not ES6 feature.

yield is actually nature to me. it means to yield to other code (other tasks in the event loop) to run first.

[zhouyx]

This is awesome! Would fix tests with callback hell and can simplify many promise related tests!
One question I have: I know in our code we use setTimeout to delay callback. are the callback in setTimeout micro task as well?
Does the setInterval work in the following scenario:

param = false
promise = new Promise(resolve => {resolver = resolve});
promise.then(() => {param = true})
doSomething = () => {setTimeout(resolver)}
doSomething();
yield
expect(param).to.be.true

[lannka]

setTimeout is not a microtask.
but your example should work. because yield flushes the eventloop, including resolver.

[zhouyx]

Hmmm I tried

it('should work ', function* () {
    let param = true;
    let resolver;
    const promise = new Promise(resolve => {resolver = resolve;});
    promise.then(() => {
      param = false;
    });
    const doSomething = () => {setTimeout(resolver);};
    doSomething();
    yield;
    expect(param).to.be.false;
  });

and it won't pass.

[jridgewell]

@zhouyx That's the thread @lannka and I have been going back and forth over. The only proper fix to this is to yield the promise (this ensure its then blocks have been called).

[lannka]

@zhouyx I added your test case into my test set, it works.

[jridgewell]

Using a done parameter prevents these tests from returning a promise as a final resolution value.

[jridgewell]

I'm going to block this until I'm overruled by @dvoytenko or @cramforce. This is going to fail in a large majority of cases (a promise chain, not just a simple, single promise). In general, this works because setInterval happens after promise resolution. But that's an internal operation and not defined by the browser.

From what I can tell, there's two motivations behind this PR:

1. Callback hell
2. Async resolution

This kind of solves callback hell. But it does nothing to guarantee async resolution happens properly (my main objection). We can use this, but we have to add a bit more code to support. @cramforce's suggestion is actually the optimal solution (so is @choumx's, because async/await can be handled by Generators), and we can use this to get there.

If we yield a promise instance, we can wait upon the promise's resolution to happen and send back the value (this is what co does). This handles callback hell (we no longer need nested then blocks), and it solves async resolution (the yielded promise is guaranteed to resolve before the test code is run again). If the promise we yield is the promise the src code is waiting on, then we get perfection.

it('co like', function* () {
  const promise = srcCode.method();
  const result = yield promise;
  expect(result).to.be(something);
});

[jridgewell]

setTimeout every time the function yields would be more appropriate. This can end up causing the test code to run again before it's really intended.

[dvoytenko]

@lannka I think this is really cool and I don't see much in terms of possibility of flakes, except for one case that I don't fully yet understand.

Imagine, this example:

it('test with promise', function* () {
  let value;
  Promise.resolve().then(function() {
    return Promise.resolve.then(function() {
      value = true;
    });
  });
  yield;
  expect(value).to.be.true;
});

This should be guaranteed to be always stable with native promises. But there's a possibility that, if a polyfill implemented via event queue (or native implemented incorrectly via event queue) we'd get the following situation:

1. First promise is scheduled as next in queue, i.e. analogous to setTimeout(0).
2. Next comes yield, it's essentially scheduled also as setTimeout(0). So it now follows (1).
3. Then the setTimeout(0) in (1) is executed and it schedules again via setTimeout(0).

So it's possible that yield will be out of sync?

My second comment - it'd be cool if yield also flushed vsync in tests. I recommend we do this via describes like this:

1. When describes instance has started, it creates a global sandbox object. We maintain a list of vsyncs = [] on the global sandbox object.
2. On yield, we go over global sandbox.vsyncs and flush them all.

[lannka]

In general, this works because setInterval happens after promise resolution.

@jridgewell no. This works because setInterval will append remaining work into the tail of the event queue. thinking it as a way to flush the event queue.

Can you come up with a test case that will fail this?

[jridgewell]

@jridgewell no. This works because setInterval will append remaining work into the tail of the event queue. thinking it as a way to flush the event queue.

That's my point. This is an internal procedure, not defined by the spec. Any promise that waits on XHR, or itself uses setTimeout (see anything that uses our Timer#promise), or any other optimization the browser makes (When do they start pausing resolution chains based on frame rate?) could break this.

@dvoytenko's code above is not defined even for the native Promise implementation. It could be result in true or false depending on the browser or polyfill.

[cramforce]

Lets make it a standard to always yield to an actual promise. We can make a helper like macroTask() that yields to the event loop like setInterval does.

[lannka]

I added tests. They run successfully under latest Chrome, Safari, FF. Saucelabs is somehow not working for me right now, so didn't check on older browsers. PTAL

@cramforce not quite get what you mean by the macroTask() helper.
@jridgewell let me know if you have any test case you think it will be flaky on some browsers.

[dreamofabear]

I think he meant you can create a Promise that resolves after a macro-task:

function macroTask() {
  return new Promise(resolve => {
    setTimeout(resolve, 0);
  });
}

You can avoid setTimeout callback hell with this.

[lannka]

verify(false);
clock.tick(99); 

setTimeout(() => {
  verify(false); 
  clock.tick(1);

  setTimeout(() => {
    verify(true);
    done();
  });
})

@choumx how would you refactor the above callback hell using your macroTask?

[dreamofabear]

return macroTask().then(() => {
  verify(false); 
  clock.tick(1);
  return macroTask();
}).then(() => {
  verify(true);
});

yield is prettier than this though. 😄

[lannka]

@choumx this seems like the same :-) just a more manual way.

Talked with @jridgewell offline. His concern is about the potential undocumented race condition between promise.resolve and setTimeout/setInterval. But in fact, without introducing this helper, the problem exists if we ever want to use setTimeout to do a later check.

verify(false);
clock.tick(100);  // this triggers a timer and then a promise chain

setTimeout(() => {
  verify(true); 
  done();
})

[jridgewell]

This would be fine:

yield macroTask();
verify(false); 
clock.tick(1);
yield macroTask();
verify(true);

That almost solves my objections because we're awaiting a promise. It's still a race condition (and so async resolution is not guaranteed) but we're at least on the right track.

The best solution is still going to be getting access to the source code's promise.

[jridgewell]

But in fact, without introducing this helper, the problem exists if we ever want to use setTimeout to do a later check.

I don't want either of those (the current or the proposed). Using setTimeout to wait on promise resolution is the reason we have flake in the current code base.

[lannka]

Using setTimeout to wait on promise resolution is the reason we have flake in the current code base.

I'd love to see a live example of those flakes. If I understand correctly, your idea is to have both promise + setTimeout to make sure remaining work is append at the end of the loop, no matter how browser treats promise and setTimeout. If so, I can add additional Promise.resolve into my setInterval to keep the neat syntax.

[jridgewell]

https://github.com/ampproject/amphtml/pull/9384/files

[lannka]

https://github.com/ampproject/amphtml/pull/9384/files

@jridgewell as we discussed offline previously, that is totally a different story. That test was flaky because the promise chain has a real XHR over the network in it. Timeout 5ms obviously isn't enough for some of the times. This test should be categorized as an integration test, where a "polling", or RequestBank (a concept I introduced to test HTTP requests) should be used.

Again, I also don't like the fact that we fixed the test by exposing an "test only" private field. Ideally, we'd keep those implementation details away from an integration test.

This introduced helper is not planed to be used in such case. If we really want to, we can change it to a unit test, where we mock out the XHR and resolve it explicitly:

xhrResolver();
yield;
check();

That way, we separated the concerns, which is the key of a unit test.

[jridgewell]

That test was flaky because the promise chain has a real XHR over the network in it.

It's using a stubbed fetch. It's flakey because the race between setTimeout and Promise resolution is not defined, as I've said over and over again. It's even using a setTimeout(..., 5) (which even more macro-task than the one proposed in this PR) and it was still flakey.

[jridgewell]

To be very specific, the problem is this:

Once execution of a Job is initiated, the Job always executes to completion. No other Job may be initiated until the currently running Job completes. However, the currently running Job or external events may cause the enqueuing of additional PendingJobs that may be initiated sometime after completion of the currently running Job. http://www.ecma-international.org/ecma-262/6.0/#sec-jobs-and-job-queues

A Job is a micro-task, and Promise#then enqueues a Job. Each browser is free to choose how they want to handle "enqueuing of additional PendingJobs that may be initiated sometime after completion of the currently running Job". The Job Queue does not have to be exhausted during this tick. So the browser does not have to evaluate an entire promise chain full of then blocks in one tick. They can choose to pause the Queue's evaluation for whatever reason.

To demonstrate in code, take my PJs Promise implementation (which AMP uses to polyfill):

// https://github.com/jridgewell/PJs/blob/b265fded45dbd06ae8a64ee0ccde29b1e3b8f760/promise.js#L423-L430
function flush() {
  // notice this rechecks `queue.length` every time,
  // meaning anything enqueued keeps it going
  for (var i = 0; i < queue.length; i++) {
    var fn = queue[i];
    queue[i] = null;
    fn(); // `fn` is a job
  }
}

I use exhaustive evaluation because it's easy, and it makes feel like promises are just as fast as nodebacks. But I could have easily done:

function flush() {
  var current = queue;
  // queue has been redefined, so anything enqueued is appended to this new array,
  // and not the one I'm currently iterating.
  queue = [];
  for (var i = 0; i < current.length; i++) {
    var fn = queue[i];
    fn();
  }
}

In this case, the queue is not exhausted and must wait till the next tick before anything the next promise in the chain is resolved. It's up to the implementation to decide on what "initiated sometime after completion" means. It's not even defined that the current job queue has to be exhausted, I could pause mid queue:

function flush() {
  // I'll arbitrarily stop at 1000 jobs,
  // because we're spending too much time here.
  // Maybe I'll implement a `performance.now` comparison to keep this under 16ms?
  // Totally up to me.
  for (var i = 0; i < 1000; i++) {
    var fn = queue[i];
    fn();
  }
  queue.splice(0, 1000);
}

With this, my problem with Async resolution is undefined. And this means the use of setTimeout to wait till after a promise chain is resolved is undefined (or racey in the best case). And this means we'd be building our test code on completely undefined behavior. That's why I'm objecting so strongly to this PR, we're codifying an undefined race condition.

If we instead yield the promise we want to wait on, we solve everything this PR wants.

1. Yield return values means callback hell go away. That's awesome.
2. The yielded promise can be waited on until it resolves, guaranteeing that race conditions are eliminated. That's even better.

[jridgewell]

We can refactor this so Promises take care of everything:

// Adapted from Babel's asyncToGenerator
return realIt(message, () => {
  return new Promise((resolve, reject) => {
    const gen = runnable();
    function step(key, arg) {
      let info;
      try {
        info = gen[key](arg);
      } catch (error) {
        reject(error);
        return;
      }

      const { value, done } = info;
      if (done) {
        resolve(value);
      } else {
        Promise.resolve(value).then(_next, _throw);
      }
    }
    function _next(value) {
      step("next", value);
    }
    function _throw(err) {
      step("throw", err);
    }
    _next();
  });
});

Differences include:

• Promises handle async (through assimilation)
• Yielded promises can reject, going into the generators throw path
• We avoid using done Forbid node-style done callback in mocha it tests #8966
• Less closures

[jridgewell]

We are we running a timeout if there's no timeout?

[lannka]

yes, i should probably document that.

[jridgewell]

We can refactor this so Promises take care of everything:

// Adapted from Babel's asyncToGenerator
return realIt(message, () => {
  return new Promise((resolve, reject) => {
    const gen = runnable();
    function step(key, arg) {
      let info;
      try {
        info = gen[key](arg);
      } catch (error) {
        reject(error);
        return;
      }

      const { value, done } = info;
      if (done) {
        resolve(value);
      } else {
        Promise.resolve(value).then(_next, _throw);
      }
    }
    function _next(value) {
      step("next", value);
    }
    function _throw(err) {
      step("throw", err);
    }
    _next();
  });
});

Differences include:

• Promises handle async (through assimilation)
• Yielded promises can reject, going into the generators throw path
• We avoid using done Forbid node-style done callback in mocha it tests #8966
• Less closures

[lannka]

just want to understand the benefit better:

Promises handle async (through assimilation)

Sorry, what does that mean? :-)

Yielded promises can reject, going into the generators throw path

This is not a problem. I should have added anyway.

We avoid using done #8966

I handled this by using a try-catch block. But in your impl, there's also a try-catch, is that there for the same reason?

Less closures

If I count correctly, all the 3 functions are closures step, _next, _throw. Not sure how it is less than the done impl.

Also, I still think there is value to support setTimeout. We will add boilerplate code to turn a setTimeout to promise.

[jridgewell]

Sorry, what does that mean? :-)

Promise assimilation is:

Promise.resolve(1).then(value => assert(value, 1));
Promise.resolve(somePromiseThatResolvesTo1).then(value => assert(value, 1));

Promises are designed to be interoperable through assimilation (they do almost exactly what you do with the value.then === function check), but in a overspecified-spec way.

I handled this by using a try-catch block

Done is actually not handled by the try-catch. If the generator threw, that's handled by it. But the problem is the final result value (when iterator.done = true) isn't waited on (it could be a promise).

But in your impl, there's also a try-catch, is that there for the same reason?

My try-catch and yours are equivalent, but neither handles the done callback (see above).

not sure how it is less than the done impl.

Multiple closures are used when the yielded value is a promise (the then blocks). This one's really a nit-pick.

Also, I still think there is value to support setTimeout

#9601 (comment)

[lannka]

I split off the changes on concurrent-load.js and test-concurrent-load-js, leaving only yield related things in this PR.

A recap of the discussions so far:

We agreed:

• Use yield promise to avoid callback hell in tests.

Still debating:

• When there is no easy way to get the promise, can we use setTimeout? I.e., can we rely on the "vaguely documented fact" that promise resolves in microtask, which queues before setTimeout.
• Whether make the impl a promise based or done based. The decision will be highly affected by the decision of the previous item.

My arguments are:

• Exposing internal promise state only for test violates test principals. The more we couple the implementation code with test code, the more technical debt we'll be paying off later on.
• Microtask seems to be a widely accepted concept. I also created tests and they all works on my local browsers. Ultimately, our unit tests are not to be run in a random polyfill.

ECMAScript has the concept of "jobs" which are similar to microtasks, but the relationship isn't explicit aside from vague mailing list discussions. However, the general consensus is that promises should be part of the microtask queue, and for good reason.
Treating promises as tasks leads to performance problems, as callbacks may be unnecessarily delayed by task-related things such as rendering. It also causes non-determinism due to interaction with other task sources, and can break interactions with other APIs, but more on that later.

I suggest we get this PR in and monitor the flakiness of the tests introduced from this PR on Travis.

@everyone thoughts?

[jridgewell]

Whether make the impl a promise based or done based. The decision will be highly affected by the decision of the previous item.

We can support the setTimeout API and remove done. It's really about duplication.

Microtask seems to be a widely accepted concept. I also created tests and they all works on my local browsers

See #9601 (comment).

Exposing internal promise state only for test violates test principals. The more we couple the implementation code with test code

I wonder if we can just accumulate all Promise instances. Something like:

it(() => {
  collectPromises();
  src.somethingThatsAsync();
  const allDone = Promises.all(stopCollectingPromises());
  yield allDone;
  // async stuff done
});

[jridgewell]

global.it = .... we could do it.yields?

[lannka]

we can do either:

it.yields('message', function*() {
});

or

it('message', function*() {
});

I'm in favor of the latter because it makes the other syntax like it.only and it.skip unchanged.

[jridgewell]

We need to wait for the state's value to complete.

if (state.done) {
  Promise.resolve(state.value).then(() => done(), done);
  return;

[lannka]

good catch

[jridgewell]

This catch clause is incorrect. If a Promise is yielded, and it rejects, it should be passed into Generator#throw. Also, it can be in the same then block, not after in a catch block:

state.value.then(_runner, (error) => {
  // this will basically duplicate everything in runner
  try {
    result = iterator.throw(error);
  } catch (e) {
    return done(e);
  }
  //...
});

[lannka]

Sorry, I still don't quite understand this part. Why is it necessary to throw the error to iterator, instead of just finish the test case immediately?

I think it's harmless to have an iterator hang there without iterating to the end, no? Or is it for better error reporting?

[jridgewell]

It's not for error reporting, but so we can take full advantage of the generator's functionality. Imagine:

it('regular code', function() {
  return somePromiseThatRejects.then(() => {
    throw new Error('UNREACHABLE');
  }, err => {
    expect(err.message).to.contain('something');
  });
});

As it is now, this cannot be written into our yield style, because the promise would reject and kill the test. Instead, we want something like:

it('generator yield', function*() {
  try {
    yield somePromiseThatRejects;
    throw new Error('UNREACHABLE');
  } catch (e) {
    expect(e.message).to.contain('something');
  }
});

That's throwing the error into the iterator, allowing us to test all our promise tests as sync generators.

[jridgewell]

Still strongly against this.

[lannka]

@jridgewell
after talking to Blink team, queuing promise resolution in microtask is a standard:
https://html.spec.whatwg.org/multipage/webappapis.html#enqueuejob(queuename,-job,-arguments)

So it's not a "undefined behavior", we should be able to use it, at least on modern browsers and well implemented polyfills.

Nevertheless, I will go with your previous suggestion, since it's more explicit and less magic behind the scene.

yield macroTask();
verify(false); 
clock.tick(1);
yield macroTask();
verify(true);

Any other concerns?

[jridgewell]

after talking to Blink team, queuing promise resolution in microtask is a standard:

I mentioned that in #9601 (comment). The undefined part is not that it's a microtask, but that running the microtasks (especially newly queued ones) isn't guaranteed in this tick.

since it's more explicit and less magic behind the scene.

Awesome. I'm on board with this, as long as it's explicit and ugly in test code. The yield 10 still makes it easy to write flakey code.

Any other concerns?

Nope, I look forward to using yield!

[lannka]

@jridgewell I've updated the code. Please take a look.

Didn't get it to return a promise because:

• so i can still do catch in for "done" state.
• one less function

[jridgewell]

One issue, then LGTM.

[jridgewell]

This is incorrect, the iterator is done and pushing the error into it won't be handled. This should be then(() => done(), done)

[lannka]

good point. fixed.

[jridgewell]

PS. Why didn't we just use async/await? We could avoid the entire it = yield(it), it.only = yield(only) stuff...

it('async', async () => {
  try {
    await somePromise;
  } catch (e) {
    // promise rejected...
  }

  const value = await anythingEvenIfItsNotAPromise;
  assert(value);
});

[lannka]

@jridgewell well, essentially we wrote a polyfill of async/await using generator to be able to run in our current project setup.

[jridgewell]

But it should just work because we compile our test code with Babel.

[lannka]

@jridgewell it does not work right away. maybe doable with preset-es2017.

But I was hesitating: is it good to have different babel setting in production vs test? Or in other words, are we ready to moving to es2017?