-
-
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
Rethink Acceptance Testing #268
Changes from 9 commits
4ec916b
f779b71
3ee1064
cdbaecb
3c57791
5e6c1bd
12f4c7b
4bb6857
a6950d9
f7656ea
8975fb9
7ee7132
1209ed3
b05ed2c
ebfdbe1
39da814
57792c2
1676701
ef325f4
6e0975c
b487cb4
8858b09
0f55cd5
8247efb
cbe88eb
6370fa8
f2b608b
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,304 @@ | ||
- Start Date: 2017-11-05 | ||
- RFC PR: [emberjs/rfcs#268](https://github.com/emberjs/rfcs/pull/268) | ||
- Ember Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
The testing story in Ember today is better than it ever has been. It is now | ||
possible to test individual component/template combos, register your own mock | ||
components/services/etc, build complex acceptance tests, and almost anything else | ||
you would like. | ||
|
||
Unfortunately, there is a massive disparity between different types of tests. | ||
In acceptance tests, you use well designed global helpers to deal with async | ||
related interactions; whereas in integration and unit tests you are forced to | ||
manually deal with this asynchrony. The goal of this RFC is to unify the | ||
concepts amongst the various types of test and provide a single common | ||
structure to tests. | ||
|
||
|
||
# Motivation | ||
|
||
Usage of rendering tests is becoming more and more common, but these tests | ||
often include manual event delegation (`this.$('.foo').click()` for | ||
example), and assumes most (if not all) interactions are synchronous. This is | ||
a major issue due to the fact that the vast majority of interactions will | ||
actually be asynchronous. There have been a few recent additions to | ||
`ember-test-helpers` that have made dealing with asynchrony better (namely | ||
[emberjs/rfcs#232](https://github.com/emberjs/rfcs/blob/master/text/0232-simplify-qunit-testing-api.md) | ||
but forcing users to manually manage all interaction based async is a recipe | ||
for disaster. | ||
|
||
Acceptance tests allow users to handle asynchrony with ease, but they rely on | ||
global helpers that automatically wrap a single global promise which makes | ||
testing of interleaved asynchronous things more difficult. There are a number | ||
of limitations in acceptance tests as compared to integration tests (cannot | ||
mock and/or stub services, cannot look up services to setup test context, etc). | ||
|
||
We need a single unified way to teach and understand testing in Ember that | ||
leverages all the things we learned with the original acceptance testing | ||
helpers that were introduced in Ember 1.0.0. Instead of inventing our own | ||
syntax for dealing with the async (`andThen`) we should use new language | ||
features such as `async` / `await`. | ||
|
||
# Detailed design | ||
|
||
The goal of this RFC is to introduce new system for acceptance tests that follows in the footsteps of | ||
[emberjs/rfcs#232](https://github.com/emberjs/rfcs/blob/master/text/0232-simplify-qunit-testing-api.md) | ||
and continues to enhance the system created in that RFC to share the same structure and helper system. | ||
|
||
This new system for acceptance tests will be implemented in the | ||
[ember-test-helpers](https://github.com/emberjs/ember-test-helpers/) library so | ||
that we can iterate faster while supporting multiple Ember versions | ||
independently and easily support multiple testing frameworks build on top of | ||
the primitives in `ember-test-helpers`. Ultimately, the existing [ember-testing](https://github.com/emberjs/ember.js/tree/master/packages/ember-testing) system | ||
will be deprecated but that deprecation will be added well after the new system has been | ||
released and adopted by the community. | ||
|
||
Lets take a look at a basic example (lifted from [the guides](https://guides.emberjs.com/v2.16.0/testing/acceptance/)): | ||
|
||
```js | ||
// **** before **** | ||
import { test } from 'qunit'; | ||
import moduleForAcceptance from '../helpers/module-for-acceptance'; | ||
|
||
moduleForAcceptance('Acceptance | posts'); | ||
|
||
test('should add new post', function(assert) { | ||
visit('/posts/new'); | ||
fillIn('input.title', 'My new post'); | ||
click('button.submit'); | ||
andThen(() => assert.equal(find('ul.posts li:first').text(), 'My new post')); | ||
}); | ||
|
||
// **** after **** | ||
import { module, test } from 'qunit'; | ||
import { setupAcceptanceTest } from 'ember-qunit'; | ||
import { visit, fillIn, click } from 'ember-test-helpers'; | ||
|
||
module('Acceptance | login', function(hooks) { | ||
setupAcceptanceTest(hooks); | ||
|
||
test('should add new post', async function(assert) { | ||
await visit('/posts/new'); | ||
await fillIn('input.title', 'My new post'); | ||
await click('button.submit'); | ||
|
||
assert.equal(find('ul.posts li')[0].textContent, 'My new post'); | ||
}); | ||
}); | ||
``` | ||
|
||
As you can see, this proposal unifies on Qunit's nested module syntax following | ||
in emberjs/rfcs#232's footsteps. | ||
|
||
## New APIs Proposed | ||
|
||
The following new methods will be exposed from `ember-qunit`: | ||
|
||
```ts | ||
declare module 'ember-qunit' { | ||
// ...snip... | ||
export function setupAcceptanceTest(hooks: QUnitModuleHooks): void; | ||
} | ||
``` | ||
|
||
### DOM Interaction Helpers | ||
|
||
New native DOM interaction helpers will be added to both `setupRenderingTest` | ||
and (proposed below) `setupAcceptanceTest`. The implementation for these | ||
helpers has been iterated on and is quite stable in the | ||
[ember-native-dom-helpers](https://github.com/cibernox/ember-native-dom-helpers) | ||
addon. | ||
|
||
The helpers will be migrated to `ember-test-helpers` and eventually | ||
(once "the dust settles") `ember-native-dom-helpers` will be able to reexport | ||
the versions from `ember-test-helpers` directly (which means apps that have | ||
already adopted will have very minimal changes to make). | ||
|
||
The specific DOM helpers to be added are: | ||
|
||
``` | ||
interface DOMInteractionHelpers { | ||
/** | ||
Clicks on the specified selector. | ||
*/ | ||
click(selector: string | HTMLElement): Promise<void>; | ||
|
||
/** | ||
Taps on the specified selector. | ||
*/ | ||
tap(selector: string | HTMLElement): Promise<void>; | ||
|
||
/** | ||
Triggers a keyboad event on the specified selector. | ||
*/ | ||
triggerKeyEvent( | ||
selector: string | HTMLElement, | ||
eventType: 'keydown' | 'keypress' | 'keyup', | ||
keyCode: string, | ||
modifiers?: { | ||
ctrlKey: false, | ||
altKey: false, | ||
shiftKey: false, | ||
metaKey: false | ||
} | ||
): Promise<void>; | ||
|
||
/** | ||
Triggers an event on the specified selector. | ||
*/ | ||
triggerEvent( | ||
selector: string | HTMLElement, | ||
eventType: string, | ||
eventOptions: any | ||
): Promise<void>; | ||
|
||
/** | ||
Fill in the specified selector's `value` property with the provided text. | ||
*/ | ||
fillIn(selector: string | HTMLElement, text: string): Promise<void>; | ||
|
||
/** | ||
Focus the specified selector. | ||
*/ | ||
focus(selector: string | HTMLElement): Promise<void>; | ||
|
||
/** | ||
Unfocus the specified selector. | ||
*/ | ||
blur(selector: string | HTMLElement): Promise<void>; | ||
|
||
/** | ||
Returns a promise which resolves when the provided callback returns a truthy value. | ||
*/ | ||
waitUntil(() => boolean): Promise<void>; | ||
|
||
/** | ||
Returns a promise which resolves when the provided selector (and count) becomes present. | ||
*/ | ||
waitFor(selector: string, count?: number): Promise<void>; | ||
} | ||
``` | ||
|
||
### `setupAcceptanceTest` | ||
|
||
This function will: | ||
|
||
* invoke `ember-test-helper`s `setupContext` with the tests context (which does the following): | ||
* 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 | ||
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. why do we need those methods? 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 main reason Once that deficiency is addressed, and we no longer need to use manual runloop wrapping during set operations in testing mode, we should consider deprecating (in favor of simply setting them as appropriate) them... 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. thanks for the explanation, but I still don't see in what situations one would use e.g. 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. Ahh, I see what you are saying now, sorry about that. I agree, I don't think we strictly need them (and I suspect 99.9% of tests will not use them anyways). I included them here so that the differences between the various types of tests are as small as possible... 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 think I'd prefer to not include them, so that people don't get the wrong idea about the things that they could use 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. Seems ok to me. We don't expect folks to need them, seems fine to avoid adding them. Will require some additional changes to the existing system in |
||
* setup `this.pauseTest` and `this.resumeTest` methods to allow easy pausing/resuming | ||
of tests | ||
* add routing related helpers | ||
* setup `this.visit` method to visit the given url | ||
* setup getter for `this.currentRouteName` which returns the current route name | ||
* setup getter for `this.currentURL` which returns the current URL | ||
* add DOM interaction helpers (heavily influenced by @cibernox's lovely addon [ember-native-dom-helpers](https://github.com/cibernox/ember-native-dom-helpers)) | ||
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 assume they will be using native DOM APIs as well (probably even reuse most of the implementation?), and not rely on jQuery anymore!? This could be stated explicitly maybe? 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. Yes, exactly. My plan (and discussed with @cibernox and @Turbo87 a couple of times) is to essentially copy the native DOM implementations from |
||
* setup a getter for `this.element` which returns the DOM element representing | ||
the applications root element | ||
* setup `this.click` helper method | ||
* setup `this.tap` helper method | ||
* setup `this.triggerKeyEvent` helper method | ||
* setup `this.triggerEvent` helper method | ||
* setup `this.fillIn` helper method | ||
* setup `this.focus` helper method | ||
* setup `this.blur` helper method | ||
* setup `this.waitUntil` helper method | ||
* setup `this.waitFor` helper method | ||
|
||
### `setupRenderingTest` | ||
|
||
The `setupRenderingTest` function proposed in | ||
[emberjs/rfcs#232](https://github.com/emberjs/rfcs/blob/master/text/0232-simplify-qunit-testing-api.md) | ||
(and implemented in | ||
[ember-qunit](https://github.com/emberjs/ember-qunit)@3.0.0) will be modified to add the same DOM interaction helpers mentioned above: | ||
|
||
* setup `this.click` helper method | ||
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 still find weird that we have two supported ways of doing the same thing. Also, specially in integration tests, it wouldn't be extraordinary that someone did
messing with the helper. It could be fixed by making Do you think that supposed improvement in debuggeablility justifies this duality? Couldn't just the docs serve as ...well... documentation of what helpers exists and how to use them? 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. FYI There is one potential advantage of using 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, however I tried to avoid talking about this in #119 (and here) because there are many other issues with running tests in parallel (there is only one DOM, focus and other events don't fire the same, etc) and I didn't want to open a can of worms. 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. Even if I slightly favour the imported helpers, my point is about having both. I think it should be one or the other. 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.
Sure, I agree that this is definitely a reasonable way to go also. I'll add a fleshed out version of it to alternatives. 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.
Right, totally understood. I guess I'm saying that the fact that there are both is an implementation detail. The primary public API and what blueprints will use will be the importable versions. The fact that they desugar to invoking methods on the test context is a minor detail (and might enable features like what @Turbo87 alludes to in #268 (comment)). 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.
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. Oh, y'all did it now. Just removed the It is still likely that during implementation I will have some mechanism to discover and access these helpers on the test context but it does not need to be public API at all... |
||
* setup `this.tap` helper method | ||
* setup `this.triggerKeyEvent` helper method | ||
* setup `this.triggerEvent` helper method | ||
* setup `this.fillIn` helper method | ||
* setup `this.focus` helper method | ||
* setup `this.blur` helper method | ||
* setup `this.waitUntil` helper method | ||
* setup `this.waitFor` helper method | ||
|
||
Once implemented, `setupRenderingTest` and `setupAcceptanceTest` will diverge from each other in very few ways. | ||
|
||
### Importable helpers | ||
|
||
Since the design of this system relies on both the test helpers being applied | ||
to the test context **and** the usage of `async` / `await`, a few importable | ||
helpers are being introduced to help avoid extra noise (e.g. "rightward shift") | ||
in tests. In most cases the imported helpers will be used, but should be | ||
considered to "de-sugar" into using the local methods on the test context. | ||
|
||
Having both mechanisms allow us to have both a clear and concise API in the | ||
simple case (by reducing rightward shift, and clarifying _where_ a given helper | ||
method is coming from), but still have the available helpers be discoverable | ||
while debugging. | ||
|
||
This means that users will be able to use either of the following interchangably: | ||
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. Do we really want/need two equivalent ways to use those helpers, or should'n there be one "right" way to do this? In the sense of strong Ember conventions, so teams don't end up bikeshedding about the right way. And maybe also useful for easier tooling (linting rules, codemods)? Just a thought... 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. @simonihmig - Yes, the blueprints will use the imported helpers. The only reason I describe both is that the importable helpers defer to the local versions on the test context (essentially as a desugaring). Do you think I need to clarify that a bit? 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 updated some verbiage in this section to hopefully clarify things a bit better. |
||
|
||
```js | ||
// ***** importable helper ***** | ||
import { click } from 'ember-test-helpers'; | ||
|
||
// ...snip... | ||
test('does something', async function(assert) { | ||
// ...snip... | ||
await click('.selector-here'); | ||
// ...snip... | ||
}); | ||
|
||
// ***** test context helper ***** | ||
|
||
// ...snip... | ||
test('does something', async function(assert) { | ||
// ...snip... | ||
await this.click('.selector-here'); | ||
// ...snip... | ||
}); | ||
``` | ||
|
||
## Changes from Current System | ||
|
||
Here is a brief list of the more important but possibly understated changes | ||
being proposed here: | ||
|
||
* The global test helpers that exist now, will no longer be present (e.g. | ||
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. Really not present, or rather deprecated? So they can co-exist, see the following section. 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. When running tests with the old 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. That makes sense! Maybe this should be stated to prevent misunderstandings like I had? |
||
`click`, `visit`, etc) and instead will be available on the test context as | ||
well as importable helpers. | ||
* `this.owner` will now be present and allow (for the first time 🎉) overriding | ||
items in the container/registry. | ||
* The new system will leverage the `Ember.Application` / `Ember.ApplicationInstance` split so that | ||
we can avoid creating an `Ember.Application` instance per-test, and instead leverage the same system | ||
that FastBoot itself uses to avoid running initializers for each acceptance test. | ||
|
||
## Migration | ||
|
||
It is important that both the existing acceptance testing system, and the | ||
newly proposed system can co-exist together. This means that new tests can be generated | ||
in the new style while existing tests remain untouched. | ||
|
||
However, it is likely that | ||
[ember-qunit-codemod](https://github.com/rwjblue/ember-qunit-codemod) will be | ||
able to accurately rewrite acceptance tests into the new format. | ||
|
||
# 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 | ||
|
||
* This is a relatively large set of changes that are arguably not needed (things mostly work today). | ||
* One of the major hurdles in upgrading larger applications to newer Ember versions, is updating their tests to follow "new" patterns. This RFC introduces yet another "new" thing (and proposes to deprecate the old thing), and could therefore be considered "just more churn". | ||
|
||
# Alternatives | ||
|
||
* Do nothing? | ||
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. An alternative (or maybe just a suggestion?) would be to make 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 question, I will add this to the alternatives section.
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 is indeed an alternative, although I think it makes more sense to make |
||
* Make `ember-native-dom-helpers` a default addon (removing the need for DOM interaction helpers proposed here). | ||
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. Looks like Keeping DOM helpers implementation as a separate package seems like a win to me from the separation of concerns standpoint. 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. @ro0gr - Forcing users to be aware of the internal packaging concerns is not ideal. From their perspective, they are using a single set of cohesive tools for testing an Ember app. Whether the implementation actually lives in Also, I disagree that |
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.
as mentioned in emberjs/ember-test-helpers#240 we might want to move to
@ember/test-helpers
for the import pathsThere 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.
Good point @Turbo87, updated in ebfdbe1.