-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Changes from 5 commits
760debe
98c3e7b
9fbe9ac
822566b
46fd4a7
9f79f62
bbd5fc6
8acc518
5ecf170
b6d26cb
c416cc0
58c0564
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,398 @@ | ||
- 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` wraps QUnit functionality makes the division | ||
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"}}`); | ||
|
||
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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the wishy washy language around arbitrarily nesting modules. 💃 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unclear to me how 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
||
* invoke `ember-test-helper`s `setContext` with the tests context | ||
* 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), but it is still useful to review concrete scenarios | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The component name is completely unused/unneeded without |
||
|
||
test('renders', async function(assert) { | ||
assert.expect(1); | ||
|
||
await this.render(hbs`{{pretty-color name="red"}}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Seems unrelated? The underlying implementation will be returning a promise. If the consuming application doesn't want to use 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
test('computed properly', function(assert) { | ||
assert.expect(1); | ||
|
||
let Factory = this.owner.factoryFor('component:x-foo'); | ||
let subject = Factory.create({ | ||
name: 'something' | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No this.subject() anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we have a public API for looking up and instantiating factories ( Removing |
||
|
||
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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point, updated! |
||
|
||
This exact process was done for `ember-mocha`'s migration, making this a well | ||
trodden path. | ||
|
||
## 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... |
There was a problem hiding this comment.
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 insteadrender
should be a the static import (liketest
,module
, etc.) or else called against some non-this
variable that can be appropriately typed, e.g. returned from thesetupRenderingTest
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
torender
orfoo.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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.