Add new QUnit testing API.

[rwjblue]

rendered

Related to (though somewhat diverged from) emberjs/ember-qunit#258.
Related to #119.
Intending to land together with #229.

[stefanpenner]

❤️

[trentmwillis]

Overall, big 👍 on this RFC. A few nitty comments to ensure clarity while reading.

One thing that did strike me, that I feels needs to be better highlighted, is that the actual testing context API is changing as well as the test module setup API. The motivation mentions changes to moduleFor but doesn't call out that other APIs will also change (e.g., this.subject -> this.owner.lookup), which I think is rather critical because it makes migration less straightforward.

[trentmwillis]

Nit: "ember-qunituses to wrap QUnit" should likely be "ember-qunit wraps QUnit"

[trentmwillis]

Nit: "invoking" -> "invoke"

[trentmwillis]

Nit: "for folks" seems unneeded here

[Turbo87]

few minor comments, but in general : yes, please!! 👍

[Turbo87]

Since when is this.render() async? What if the platform doesn't support async/await yet?

[rwjblue]

Since when is this.render() async?

Rendering a template has actually never been guaranteed to be synchronous. Future iterations of things in glimmer-vm will almost certainly take more advantage of async during rendering to allow the rendering engine to yield back to the browser to avoid blocking the main thread. Making this.render be a promise based API is a way to help ensure a change (that would help all apps in reality) doesn't troll all of our tests.

What if the platform doesn't support async/await yet?

Seems unrelated? The underlying implementation will be returning a promise. If the consuming application doesn't want to use async/await it would use promise chaining.

  test('renders', async function(assert) {
    assert.expect(1);

    return this.render(hbs`{{pretty-color name="red"}}`)
      .then(() => {
        assert.equal(this.$('.color-name').text(), 'red');
      });
  });

[Turbo87]

IMHO we should pass the component name to the setup function here instead of relying on the module name, which seems a little brittle

[rwjblue]

The component name is completely unused/unneeded without needs/unit options (which this API will not support). There is no reliance on the module name at all either.

[Turbo87]

same as above. how does setupTest() know that we're talking about a component and what its name is?

[rwjblue]

It doesn't matter at all actually. Thats the beauty of removing the arbitrary resolver restrictions (in #229).

There will be a number of changes to the primitives in ember-test-helpers to support this nicely (for both ember-mocha and ember-qunit).

[Turbo87]

No this.subject() anymore?

[rwjblue]

No, we have a public API for looking up and instantiating factories (factoryFor), which is polyfilled nicely via ember-getowner-polyfill and ember-factory-for-polyfill.

Removing this.subject and the unit / needs options is how we avoid requiring the extra arguments to setupTest.

[Turbo87]

Might be worth mentioning that we are already doing the same thing for ember-mocha too

[rwjblue]

Great point, updated!

[Turbo87]

so this is actually not an issue since we can just bump the QUnit dependency in ember-qunit right?

[rwjblue]

Confirm. I've spoken to @trentmwillis about it also, and I believe there shouldn't be a problem with getting a release out by the time this RFC is ready to be merged (which is a week or two at the minimum).

[pittst3r]

It's unclear to me how setupTest, etc. will be able to access the tests' this when we are only passing in the hooks object. To illustrate what I'm talking about, this is what I am doing in Glimmer:

function setupRenderingTest(context, hooks) {
  context.render = function render(/* not important */) {
    // ...
  };

  // ...
}
module('Component: <hello-glimmer />', function(hooks) {
  setupRenderingTest(this, hooks);

  test('it renders', assert => {
    // ...
  });
});

Basically just wondering if the setupTest(hooks) signature is realistic or if it's going to need to be setupTest(this, hooks). Or maybe this is a detail beyond the scope of this RFC.

[rwjblue]

@robbiepitts - A basic implementation of this API is already done (see here), and does function properly without the context being passed in. I believe that the signature proposed here is realistic.

[pittst3r]

Ohhh, cool. I didn't think of doing it that way.

[cursedquail]

Nested hooks would be amazing for more complex components / routes / whatever.

An example would be a route that has different behavior for "admin" users. Both of them need some similar setup (e.g. setting up mirage), but the two different "nested" modules could authenticate two different users - one admin and one not.

Currently not supporting nested modules is a bit of a pain point for our current apps, and I would love to have the ability to nest modules. What would be the blocker for supporting this?

[rwjblue]

@NLincoln:

I'll try to update the wording to be a bit more clear. What I'm saying is that this RFC isn't saying that you must use multiple levels of nesting (that seems like an application level stylistic choice), but it is requiring that we use the nested modules syntax even if only using a single level.

This RFC proposal (and the spike implementation done in emberjs/ember-qunit#258) would fully support QUnit nested modules.

[rwjblue]

I removed the wishy washy language around arbitrarily nesting modules. 💃

[cursedquail]

Yessssss.... 😄 Very excited about this 👍

[rwjblue]

@trentmwillis:

One thing that did strike me, that I feels needs to be better highlighted, is that the actual testing context API is changing as well as the test module setup API.

@Turbo87:

IMHO we should pass the component name to the setup function here instead of relying on the module name, which seems a little brittle
...
Since when is this.render() async?

Good points! I've just added a section to specifically call out significant changes from the current system.

[hjdivad]

hugely excited for this: 👍

[rwjblue]

A few folks (e.g. @ebryn and @stefanpenner) have approached me with concerns around the hooks argument I have mentioned/used here. The concerns are generally an initial reaction to the QUnit nested modules API in general and not directly related to this RFC (other than it highlighting a new feature that they haven't used before).

The main concerns as I understood it are:

• Teaching folks what hooks means is a bit more difficult because it does not represent the "test environment", but rather just a way to invoke the callbacks for before / beforeEach / after / afterEach.
• Passing only hooks to the helper functions proposed in the RFC means that if we ever need to thread more information through, we either have to use hooks as a transport or change our API to add more arguments.
• It seems somewhat impossible to communicate across multiple helpers (again without using hooks as a state/transport mechanism).

I've kicked off a conversation over with the QUnit folks in qunitjs/qunit#1200. If that PR were merged this proposal would be modified to the following syntax:

// current proposal
module('x-foo', function(hooks) {
  setupRenderingTest(hooks);
  // ....snip....
});

// after qunitjs/qunit#1200
module('x-foo', function(hooks) {
  setupRenderingTest(this);
  // ....snip....
});

---

I will update the RFC to list this as an unanswered question, but I personally do not find this to be a blocking issue (we'll see how others feel). This RFC is mostly just leveraging what the underlying test framework has to offer (which is one of the primary objectives in the motivation section).

[Turbo87]

@rwjblue I'm not sure I understand that last comment about hooks properly yet. In ember-mocha we don't need to pass this into the setup functions at all since we only need this in the setup/teardown functions, which are bound to the correct context automatically... maybe I'm misunderstanding something here 🤔

[rwjblue]

@Turbo87:

In ember-mocha we don't need to pass this into the setup functions at all since we only need this in the setup/teardown functions, which are bound to the correct context automatically

Yep, I absolutely understand how it works in ember-mocha. Unlike mocha, QUnit does not provide global functions for before/beforeEach/afterEach/after. The only way to register these callbacks in a QUnit setting is to pass in the first argument of the module callback (which has been named hooks in the examples for this RFC as well as a few examples on qunitjs.com).

[rwjblue]

Another possible mitigation to the strangeness of the word hooks as the argument, is to use module. After discussing with the QUnit folks, it seems that this argument represents the "module context".

This would change the examples here in this RFC to be:

module('x-foo', function(module) {
  setupRenderingTest(module);
  // ....snip....
});

The mental model here would be:

The callback receives the module instance that was created so that you can add before/beforeEach/afterEach/after callbacks.

tldr; I think module works much better here...

[rwjblue]

This was discussed at the 2017-06-30 core team meeting, and given that the responses by folks are overwhelmingly positive it is ready to advance to final comment period.

[rwjblue]

QUnit 2.4.0 was just released (thanks @trentmwillis!) which includes the required changes to enable this API.

[rwjblue]

No new information was identified during final comment period, thanks to everyone for the detailed review and help working through this. Let's get it done!