Generalize waiting for asynchronous behaviour in testing

[cibernox]

I write this as an issue instead of as a full blown RFC because I might no be needed, but
I'm happy to escalate it if feels that deserves more official discussion.

The problem

This problem arises from my effors to allowing async/await in testing to actually work as
expected in the Grand Testing Unification and also decoupling Ember from jQuery.

ember-native-dom-helpers makes integrations
and acceptance testing almost usable, but there is a very important edge case that doesn't
support, and neither does the andThen helper:

Waiting for asynchronous behaviour not happenig inside route's hooks.

I thought that andThen and the wait() function from ember-test-helpers waited for all
promises to be resolved, but that is not the case. They wait for:

• All timers scheduled with Ember.run.later to be fired.
• All queues of the runloop to be drained.
• All route hooks to settle.
• All jQuery ajax requests to settle.

Therefore, that means we cannot test at the moment, with any approach, things like:

export default Ember.Controller.extend({
  actions: {
    submit(data) {
      fetch('/somewhere', { method: 'POST', data }).then(() => this.showGreenFlash());
    }
  }
})

The reason why people rarely complain about this is because the big mayority of the community
uses ember-ajax or Ember.$, and jQuery requests are tracked using the ajaxSend and ajaxComplete events [see code]

This problem is/will be also in glimmer.

What do we need

We need a way to wait for async behaviour (usually network, but not exclusively) in testing. Ideally it should be:

• Be free in production and at least cheap in development.
• Do not rely on any particular promise implementation (RSVP).
• Work inside and outside of route hooks.
• Make it as transparent to users as possible, but still allow them to do use it for waiting
  unconventional events, like waiting until a websockets receive a message.

Proposed approach

I propose a couple of low level utility functions, probably living in ember-test-helpers, that behave
similarly to defer/advanceReadiness in initializers. One of those functions increments a
global counter of "things being waited on" and another function decrements it.

The andThen and wait() helpers will wait until the counter of waiters to be zero.

Usage example:

// components/slack-widget.js
import { deferTestingReadiness, advanceTestingReadiness } from 'ember-test-helpers';

export default Ember.Controller.extend({
  websocketChannel: Ember.Service.inject(),
  init() {
    this._super(...arguments);
    this.get('websocketChannel').join('#-dev-testing');
    deferTestingReadiness();
    this.get('websocketChannel').listen(function(msg) {
      if (msg.type === 'joined') { advanceTestingReadiness(); }
    });
  }
});

This would allow to test this as:

test('can annoy @rwjblue', async function() {
  await visit('/channels-hub');

  await click('.channel-dev-testing')
  assert.ok(find('.successful-join-flash'), 'The user joined the channel');
});

It might worth having a slightly less low level convenience function that handles promises
and it is not tied to any RSVP specific feature ("Use the platform" and all that jazz).

import { trackPromise } from 'ember-test-helpers';

export default Ember.Controller.extend({
  actions: {
    submit(data) {
      trackPromise(fetch('/somewhere', { method: 'POST', data }).then(() => this.showGreenFlash()));
    }
  }
})

The implementation of that track promise function would just be:

function trackPromiseFinish(result) {
  advanceTestingReadiness();
  return result;
}
export function trackPromise(thenable) {
  deferTestingReadiness();
  return thenable.then(trackPromiseFinish, trackPromiseFinish);
}

How to make users mostly unaware of this

Wrapping every async on this just for testing purposes is pretty annoying but there isn't
really any generic way of knowing when async stuff have finished. The closest thing is using
some obscure RSVP instrumentation and still that doesn't cover other uses like websockets, browser timers (no run.later), posting messages to web workers and awaiting their response.

This has always been broken and people didn't complain that much because jquery AJAX
was covered, so to me it shows that just by covering requests we are accounting for almost all
usecases people might have.

If we make PRs to the most popular networking addons in the wild (ember-fetch, ember-network, ember-ajax) most people will never know that this exists.

Open questions.

• Where should this helper live? ember-test-helpers?
• This code should be stripped in production. Would be runInDebug good enough? Does it
  remove dead code and unused imports?

import { trackPromise } from 'ember-test-helpers';

export default Ember.Controller.extend({
  websocketChannel: Ember.Service.inject(),
  init() {
    this._super(...arguments);
    this.get('websocketChannel').join('#-dev-testing');
    runInDebug(deferTestingReadiness)
    this.get('websocketChannel').listen(function(msg) {
      if (msg.type === 'joined') {
        runInDebug(advanceTestingReadiness)
      }
    });
  },

  actions: {
    submit(data) {
      let promise = fetch('/somewhere', { method: 'POST', data }).then(() => this.showGreenFlash());
      runInDebug(() => trackPromise(promise))
    }
  }
})

could be removed into:

// import statement must be gone

export default Ember.Controller.extend({
  websocketChannel: Ember.Service.inject(),
  init() {
    this._super(...arguments);
    this.get('websocketChannel').join('#-dev-testing');
    this.get('websocketChannel').listen(function(msg) {
      if (msg.type === 'joined') { // empty branch should ideally be removed
      }
    });
  },

  actions: {
    submit(data) {
      // unused variable declaration should be removed
      let promise = fetch('/somewhere', { method: 'POST', data }).then(() => this.showGreenFlash());
    }
  }
})

[cibernox]

/cc @rwjblue as proposer of the appoach
/cc @stefanpenner as Mr. Promises

[Turbo87]

@cibernox I'm not a big fan of the example given in "Proposed approach". IMHO those testing hooks should only exist/be called in dev/test mode, but not in production which the example seems to suggest. I'd rather see the existing networking addons provide some sort of hook that can be enabled/implemented by the test helpers or the test adapter in Ember.

[cibernox]

@Turbo87 I agree that they should only be called in testing, not in dev/productio (I'd be ok if they are called in dev, not a big deal).
That is why I suggest to wrap every usage runInDebug, because it should be stripped from the production build with a babel transform, much like ember-test-selectors do, including removing import statements.

[jgwhite]

This may be relevant:

#119 (comment)

(TL;DR copy Capybara’s approach)

[cibernox]

@jgwhite This is where we head. That is where we are already if we use jQuery, but people (read "me") want to use fetch.

[jgwhite]

@cibernox I’m not referencing the whole RFC, just @machty’s sub-proposal.

[cibernox]

@jgwhite I know, I already do that (different syntax. Using an independent waitUntil instead of making finders async): https://github.com/cibernox/ember-native-dom-helpers#testing-an-unsettled-world. It works as long as you use jquery.

[jgwhite]

@cibernox thanks for clarifying 👍

[hjdivad]

@cibernox these utility functions can be implemented today in an addon by utilizing registerWaiter, no?

[hjdivad]

@cibernox these utility functions can be implemented today in an addon by utilizing registerWaiter, no?

Also, although it can be a pain to do so for each kind of async, using waiters for things like fetch obviates the need for code stripping.

[rwjblue]

@hjdivad:

these utility functions can be implemented today in an addon by utilizing registerWaiter, no?

yep, but the idea here is to get "buy in" to using a shared addon

although it can be a pain to do so for each kind of async, using waiters for things like fetch obviates the need for code stripping

How so? ember-fetch / ember-network would still have to implement their own Ember.Test.registerWaiter, and then either guard (and leave code in for prod) or strip...

See liquid-fire's transition-map (which is guarded to only setup when `isTesting) for an example of this...

[cibernox]

@hjdivad registerWaiter is part of the picture but not the bit I want to add.

Using registerWaiter we can command andThen and wait to wait for until some condition is true. That works as a charm.

The problem is that if we don't track pending promises, like fetch requests. What I propose is add conventional utility functions to track async stuff in testing instead of leaving everyone come up with their own approach.

The alternative is that ember-fetch register its own waiter, ember-network register it's own waiter, liquid-fire too, etc...

That is doable, but I thought that a unified utility could be better.

The idea behind having a single utility is that we can put more effort in making smarter (p.e. a babel-tranform that removes that logic in production). If I was the author of ember-fetch I'd rather not have to have to worry about that.

[cibernox]

Also, I think that that network requests is something we always what to wait for, so I'd add that waiter by default.

[hjdivad]

@rwjblue

How so? ember-fetch / ember-network would still have to implement their own Ember.Test.registerWaiter, and then either guard (and leave code in for prod) or strip...

By registering the waiter only in tests, and mocking the underlying API only in tests (in cases where you can't hook into before/after events, as with something like $.ajax)

---

@cibernox

The alternative is that ember-fetch register its own waiter, ember-network register it's own waiter, liquid-fire too, etc..

Yes but isn't that better than users needing to remember to call something at every async point? Installing something like ember-fetch (and for ember-fetch to register and manage a waiter) is something a user would do once per type of async, but with

  actions: {
    submit(data) {
      let promise = fetch('/somewhere', { method: 'POST', data }).then(() => this.showGreenFlash());
      runInDebug(() => trackPromise(promise))
    }
  }

Calling trackPromise is something the user has to remember to do at every async call.

Also, I think that that network requests is something we always what to wait for, so I'd add that waiter by default.

Yes, I absolutely agree. There seems to be a tradeoff between needing to write many waiters or imposing a burden on the user at async call sites. Do you agree this is the tradeoff?

[cibernox]

@hjdivad I don't indent this to be usually done by users. Indeed, ember-fetch and ember-network should use the utility functions I propose internally.
That is what I wanted to express with

If we make PRs to the most popular networking addons in the wild (ember-fetch, ember-network, ember-ajax) most people will never know that this exists.

All this work can be done without touching ember's core since it's not giving the users new capabilities but exposing a conventional way (for library authors mostly) to register something all tests should wait for, and probably some build-pipeline magic to strip all that in production. ember-test-helpers seems a good place for this.

[hjdivad]

@hjdivad I don't indent this to be usually done by users. Indeed, ember-fetch and ember-network should use the utility functions I propose internally.

Ah okay, I see. Thanks for clarifying.

All this work can be done without touching ember's core since it's not giving the users new capabilities but exposing a conventional way (for library authors mostly) to register something all tests should wait for, and probably some build-pipeline magic to strip all that in production.

Makes sense. My feeling is still that this already kind of exists, between registering a waiter and putting the registration code within test support.

[rwjblue]

My feeling is still that this already kind of exists, between registering a waiter and putting the registration code within test support.

I'm not sure how things like ember-fetch or liquid-fire could put the registration code in test support. The addons addon-test-support (or test-support but don't use that 😝 ) don't get invoked during a test run unless the app kicks it off. As you can see from my links above (in #218 (comment)) each library would generally have to leave this code in place in prod builds. It is tiny but still somewhat annoying.

---

Regardless, it seems that we all are in violent agreement that fetch calls should be automatically "pausing" tests. I submitted ember-cli/ember-fetch#25 to get the ball rolling...

[cibernox]

@rwjblue @hjdivad I've decided to start with a simplification of this RFC. I thought that "network" requests, originated from $.ajax or by any other mean (xhr, fetch...) should all be tracked together.

[cibernox]

@hjdivad emberjs/ember-test-helpers#202

[rwjblue]

I believe that the majority of the asks in this issue have been addressed, specifically waitUntil, the usage of ember-test-waiters (which provides a handy waitForPromise API) internally to @ember/test-helpers.

@cibernox - Would you mind reviewing to see if we can close this one?

[wagenet]

I'm closing this due to inactivity. This doesn't mean that the idea presented here is invalid, but that, unfortunately, nobody has taken the effort to spearhead it and bring it to completion. Please feel free to advocate for it if you believe that this is still worth pursuing. Thanks!