Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new QUnit testing API. #232

Merged
merged 12 commits into from Jul 29, 2017
395 changes: 395 additions & 0 deletions text/0230-simplify-qunit-testing-api.md
@@ -0,0 +1,395 @@
- Start Date: 2017-06-13
- RFC PR: [emberjs/rfcs#232](https://github.com/emberjs/rfcs/pull/232)
- Ember Issue: (leave this empty)

# Summary

In order to embrace newer features being added by QUnit (our chosen default
testing framework), we need to reduce the brittle coupling between `ember-qunit`
and QUnit itself.

This RFC proposes a new testing syntax, that will expose QUnit API directly while
also making tests much easier to understand.

# Motivation

QUnit feature development has been accelerating since the ramp up to QUnit 2.0.
A number of new features have been added that make testing our applications
much easier, but the current structure of `ember-qunit` impedes our ability
to take advantage of some of these features.

Developers are often confused by our `moduleFor*` APIs, questions like these
are very common:

* What "magic" is `ember-qunit` doing?
* Where are the lines between QUnit and ember-qunit?
* How can I use QUnit for plain JS objects?

The way that `ember-qunit` uses to wrap QUnit functionality makes the division
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

of responsiblity much harder to understand, and leads folks to believe that there
is much more going on in `ember-qunit` than there is. It should be much clearer
what `ember-qunit` is responsible for and what we rely on QUnit for.

When this RFC has been implemented and rolled out, these questions should all be
addressed and our testing system will both: embrace QUnit much more **and**
be much more framework agnostic. Sounds like a neat trick, huh?

# Detailed design

The primary change being proposed in this RFC is to migrate to using the QUnit
nested module syntax, and update our custom setup/teardown into a more functional
API.

Lets look at a basic example:

```js
// **** before ****

import { moduleForComponent, test } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';

moduleForComponent('x-foo', {
integration: true
});

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

this.render(hbs`{{pretty-color name="red"}}`);

assert.equal(this.$('.color-name').text(), 'red');
});

// **** after ****

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';

module('x-foo', function(hooks) {
setupRenderingTest(hooks);

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

await this.render(hbs`{{pretty-color name="red"}}`);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue apologies for wandering by, but seems like continuing setupRenderingTest-injects-render-into-this is continuing the "change objects at runtime" style of ember's JS roots.

If a goal (of some subset of the users/disclaimer IANAE) is to have better TS support, it seems like these sort of "suddenly show up on this" methods will be inherently untypeable, and instead render should be a the static import (like test, module, etc.) or else called against some non-this variable that can be appropriately typed, e.g. returned from the setupRenderingTest method/something like that.

Granted, no idea what compromises you have to make for backwards compatibility, e.g. if having users move from this.render to render or foo.render is just a no-go, but if you're making a big change, seems like moving towards typeable-if-you-want APIs/DSLs would be worthwhile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subsequent to this RFC I implemented the helpers as importables (in fact that is what the codemod actually does), there is more written on this in #268.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great, thanks! I'd read #268 and noticed it was that way there, but didn't put 2+2 together to realize that meant these were older examples that were supplanted by #268's.


assert.equal(this.$('.color-name').text(), 'red');
});
});
```

As you can see, this proposal leverages QUnit's nested module API in a way that
makes it much clearer what is going on. It is quite obvious what QUnit is doing
(acting like a general testing framework) and what `ember-qunit` is doing
(setting up rendering functionality).

This API was heavily influenced by the work that
[Tobias Bieniek](https://github.com/Turbo87) did in
[emberjs/ember-mocha#84](https://github.com/emberjs/ember-mocha/pull/84).

## QUnit Nested Modules API

Even though it is not a proposal of this RFC, the QUnit nested module
syntax may seem foreign to some folks so lets briefly review.

With nested modules, a normal 1.x QUnit module setup changes from:

```js
QUnit.module('some description', {
before() {},
beforeEach() {},
afterEach() {},
after() {}
});

QUnit.test('it blends', function(assert) {
assert.ok(true, 'of course!');
});
```

Into:

```js
QUnit.module('some description', function(hooks) {

hooks.before(() => {});
hooks.beforeEach(() => {});
hooks.afterEach(() => {});
hooks.after(() => {});

QUnit.test('it blends', function(assert) {
assert.ok(true, 'of course!');
});
});
```

This makes it much simpler to support multiple `before`, `beforeEach`, `afterEach`,
and `after` callbacks, and it also allows for arbitrary nesting of modules (though
this RFC is not advocating for arbitrary levels of nesting).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yessssss.... 😄 Very excited about this 👍


You can read more about QUnit nested modules
[here](http://api.qunitjs.com/QUnit/module#nested-module-nested-hooks-). The new APIs
proposed in this RFC expect to be leveraging nested modules.

## New APIs

The following new methods will be exposed from `ember-qunit`:

```ts
interface QUnitModuleHooks {
before(callback: Function): void;
beforeEach(callback: Function): void;
afterEach(callback: Function): void;
after(callback: Function): void;
}

declare module 'ember-qunit' {
// ...snip...
export function setupTest(hooks: QUnitModuleHooks): void;
export function setupRenderingTest(hooks: QUnitModuleHooks): void;
}
```

### `setupTest`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


This function will:

* invoking `ember-test-helper`s `setContext` with the tests context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "invoking" -> "invoke"

* create an owner object and set it on the test context (e.g. `this.owner`)
* setup `this.set`, `this.setProperties`, `this.get`, and `this.getProperties` to
the test context
* setup `this.pauseTest` and `this.resumeTest` methods to allow easy pausing/resuming
of tests

### `setupRenderingTest`

This function will:

* run the `setupTest` implementation
* setup `this.$` method to run jQuery selectors rooted to the testing container
* setup a getter for `this.element` which returns the DOM element representing
the element that was rendered via `this.render`
* setup Ember's renderer and create a `this.render` method which accepts a
compiled template to render and returns a promise which resolves once rendering
is completed
* setup `this.clearRender` method which clears any previously rendered DOM (
also used during cleanup)

When invoked, `this.render` will render the provided template and return a
promise that resolves when rendering is completed.


## Migration Examples

The migration can likely be largely automated (following the
[excellent codemod](https://github.com/Turbo87/ember-mocha-codemods) that
[Tobias Bieniek](https://github.com/turbo87) wrote for a similar `ember-mocha`
the transition), for folks but it is still useful to review concrete scenarios
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "for folks" seems unneeded here

of tests before and after this RFC is implemented.

### Component / Helper Integration Test

```js
// **** before ****

import { moduleForComponent, test } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';

moduleForComponent('x-foo', {
integration: true
});

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

this.render(hbs`{{pretty-color name="red"}}`);

assert.equal(this.$('.color-name').text(), 'red');
});

// **** after ****

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';

module('x-foo', function(hooks) {
setupRenderingTest(hooks);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@rwjblue rwjblue Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

await this.render(hbs`{{pretty-color name="red"}}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
      });
  });


assert.equal(this.$('.color-name').text(), 'red');
});
});
```

### Component Unit Test

```js
// **** before ****

import { moduleForComponent, test } from 'ember-qunit';

moduleForComponent('x-foo', {
unit: true
});

test('computes properly', function(assert) {
assert.expect(1);

let subject = this.subject({
name: 'something'
});

let result = subject.get('someCP');
assert.equal(result, 'expected value');
});

// **** after ****

import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';

module('x-foo', function(hooks) {
setupTest(hooks);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


test('computed properly', function(assert) {
assert.expect(1);

let Factory = this.owner.factoryFor('component:x-foo');
let subject = Factory.create({
name: 'something'
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this.subject() anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


let result = subject.get('someCP');
assert.equal(result, 'expected value');
});
});
```

### Service/Route/Controller Test

```js
// **** before ****

import { moduleFor, test } from 'ember-qunit';

moduleFor('service:flash', 'Unit | Service | Flash', {
unit: true
});

test('should allow messages to be queued', function (assert) {
assert.expect(4);

let subject = this.subject();

subject.show('some message here');

let messages = subject.messages;

assert.deepEqual(messages, [
'some message here'
]);
});

// **** after ****

import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';

module('Unit | Service | Flash', function(hooks) {
setupTest(hooks);

test('should allow messages to be queued', function (assert) {
assert.expect(4);

let subject = this.owner.lookup('service:flash');

subject.show('some message here');

let messages = subject.messages;

assert.deepEqual(messages, [
'some message here'
]);
});
});

```

## Ecosystem Updates

The blueprints in all official projects (and any provided by popular addons)
will need to be updated to detect `ember-qunit` version and emit the correct
output.

This includes:

* ember-source
* ember-data
* ember-cli-legacy-blueprints
* others?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, updated!


## Update Guides

The guides includes a section for testing, this section needs to be reviewed
and revamped to match the proposal here.

## Deprecate older APIs

Once this RFC is implemented, the older APIs will be deprecated and retained
for a full LTS cycle (assuming speedy landing, this would mean the older APIs
would be deprecated around Ember 2.20). After that timeframe, the older APIs
will be removed from `ember-qunit` and `ember-test-helpers` and they will
release with SemVer major version bumps.

Note that while the older `moduleFor` and `moduleForComponent` APIs will be
deprecated, they will still be possible to use since the host application can
pin to a version of `ember-qunit` / `ember-test-helpers` that support its own
usage. This is a large benefit of migrating these testing features away from
`Ember`'s internals, and into the addon space.

## Relationship to "Grand Testing Unification"

This RFC is a small stepping stone towards the future where all types of tests
share a similar API. The API proposed here is much easier to extend to provide
the functionality that is required for [emberjs/rfcs#119](https://github.com/emberjs/rfcs/pull/119).

# How We Teach This

This change requires updates to the API documentation of `ember-qunit` and the
main Ember guides' testing section. The changes are largely intended to reduce
confusion, making it easier to teach and understand testing in Ember.

# Drawbacks

## Churn

As mentioned in [emberjs/rfcs#229](https://github.com/emberjs/rfcs/pull/229), test
related churn is quite painful and annoying. In order to maintain the general
goodwill of folks, we must ensure that we avoid needless churn.

This RFC should be implemented in conjunction with
[emberjs/rfcs#229](https://github.com/emberjs/rfcs/pull/229) so that we can avoid
multiple back to back changes in the blueprints.

## [qunitjs/qunit#977](https://github.com/qunitjs/qunit/issues/977)

Until very recently, the QUnit nested module API was only able to allow a single
callback for each of the hooks per-nesting level. This means that the proposal in
this RFC (which requires the hooks to be setup by `ember-qunit`) would disallow
user-land `beforeEach`/`afterEach` hooks to be setup.

The work around is "simple" (if somewhat annoying), which is to "just nest another
level". The good news is that [Trent Willis](https://github.com/trentwillis) fixed
the underlying problem in [qunitjs/qunit#1188](https://github.com/qunitjs/qunit/pull/1188),
which should be released as 2.3.4 well before this RFC is merged.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


# Alternatives

The simplest alternative is to do nothing. This would loose all of the positive
benefits mentioned in this RFC, but should still be considered a possibility...