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

Rethink Acceptance Testing #268

Merged
merged 27 commits into from
Dec 1, 2017
Merged
Changes from 17 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4ec916b
Rethink Acceptance Testing
rwjblue Nov 6, 2017
f779b71
Tweaks/corrections.
rwjblue Nov 6, 2017
3ee1064
Tweak alternative.
rwjblue Nov 6, 2017
cdbaecb
Fix typo in intro of detailed design.
rwjblue Nov 6, 2017
3c57791
Better document the DOM Helpers in the detailed design.
rwjblue Nov 6, 2017
5e6c1bd
Clarify that `root element` meant the applications root element.
rwjblue Nov 6, 2017
12f4c7b
Remove `this.$` helper.
rwjblue Nov 6, 2017
4bb6857
Clarify the reasoning for both helper types.
rwjblue Nov 6, 2017
a6950d9
Add alternative about leaving ember-native-dom-helpers.
rwjblue Nov 6, 2017
f7656ea
Remove reference to `this.set` / `this.setProperties` / etc.
rwjblue Nov 6, 2017
8975fb9
Remove public API suggestion for DOM interaction methods.
rwjblue Nov 6, 2017
7ee7132
Remove more `this.foo` references.
rwjblue Nov 6, 2017
1209ed3
Add blurb about removing implicit promise chaining.
rwjblue Nov 6, 2017
b05ed2c
Tweak summary prose for clarity and purpose.
rwjblue Nov 8, 2017
ebfdbe1
Migrate to use @ember/test-helpers.
rwjblue Nov 8, 2017
39da814
Remove wrapping `declare module` statement.
rwjblue Nov 8, 2017
57792c2
Add mention of removing default `tests/helpers/*.js`.
rwjblue Nov 9, 2017
1676701
Correct the options to `waitFor` and `waitUntil`.
rwjblue Nov 10, 2017
ef325f4
Add example of addon supporting both APIs.
rwjblue Nov 10, 2017
6e0975c
Remove reference to `find` in example.
rwjblue Nov 10, 2017
b487cb4
Add example demonstrating factory overrides.
rwjblue Nov 10, 2017
8858b09
Add more details about waiting for the result of `click` et al
rwjblue Nov 10, 2017
0f55cd5
Fix typos in test helper example.
rwjblue Nov 10, 2017
8247efb
Remove extraneous option for test helpers.
rwjblue Nov 10, 2017
cbe88eb
Update typings for `waitFor` and `waitUntil`.
rwjblue Nov 11, 2017
6370fa8
Add type for code snippet.
rwjblue Nov 15, 2017
f2b608b
Rename import to `setupApplicationTest`.
rwjblue Nov 15, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
278 changes: 278 additions & 0 deletions text/0268-acceptance-testing-refactor.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,278 @@
- 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.
[emberjs/rfcs#232](https://github.com/emberjs/rfcs/blob/master/text/0232-simplify-qunit-testing-api.md)
introduced us to QUnit's nested modules API, made integration and unit testing
modular, and greatly simplified the concepts needed to learn how to write unit
and integration tests. The goal of this RFC is to leverage what we have learned
in prior RFCs and apply that knowledge to acceptance testing. Once this RFC has
been implemented all test types in Ember will have a unified cohesive structure.

# 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 to the `@ember/test-helpers` module are:

```
/**
Clicks on the specified selector.
*/
export function click(selector: string | HTMLElement): Promise<void>;

/**
Taps on the specified selector.
*/
export function tap(selector: string | HTMLElement): Promise<void>;

/**
Triggers a keyboad event on the specified selector.
*/
export function 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.
*/
export function triggerEvent(
selector: string | HTMLElement,
eventType: string,
eventOptions: any
): Promise<void>;

/**
Fill in the specified selector's `value` property with the provided text.
*/
export function fillIn(selector: string | HTMLElement, text: string): Promise<void>;

/**
Focus the specified selector.
*/
export function focus(selector: string | HTMLElement): Promise<void>;

/**
Unfocus the specified selector.
*/
export function blur(selector: string | HTMLElement): Promise<void>;

/**
Returns a promise which resolves when the provided callback returns a truthy value.
*/
export function waitUntil(() => boolean): Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

export function waitUntil(() => boolean, { timeout = 1000 }): Promise<void>;


/**
Returns a promise which resolves when the provided selector (and count) becomes present.
*/
export function waitFor(selector: string, count?: number): Promise<void>;
Copy link
Contributor

@cibernox cibernox Nov 10, 2017

Choose a reason for hiding this comment

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

export function waitFor(selector: string, { timeout = 1000, count?: number }): Promise<void>;

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 catch, updated!

```

### `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.pauseTest` and `this.resumeTest` methods to allow easy pausing/resuming
of tests
* add routing related helpers
* setup importable `visit` method to visit the given url
* setup importable `currentRouteName` method which returns the current route name
* setup importable `currentURL` method 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))
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ember-native-dom-helpers (the addons own README infers that it is specifically aiming for this).

* setup a getter for `this.element` which returns the DOM element representing
the applications root element
* setup importable `click` helper method
* setup importable `tap` helper method
* setup importable `triggerKeyEvent` helper method
* setup importable `triggerEvent` helper method
* setup importable `fillIn` helper method
* setup importable `focus` helper method
* setup importable `blur` helper method
* setup importable `waitUntil` helper method
* setup importable `waitFor` helper method
Copy link
Contributor

Choose a reason for hiding this comment

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

  • waitFor doesn't exist in ember-native-dom-helpers. So you like to have it added prior to the migration?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you haven't included find and findAll intentionally, with the idea that that users can just "use the platform". However, specially find compared to this.element.querySelector() is much more terse. Same goes for findAll vs this.element.querySelectorAll(), with the extra annoyance that querySelectorAll returns a NodeList instance instead of an Array, which is cumbersome to use to say the list.

I'd consider adding find and findAll. I believe that people will end up using them anyway.


### `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 importable `click` helper method
* setup importable `tap` helper method
* setup importable `triggerKeyEvent` helper method
* setup importable `triggerEvent` helper method
* setup importable `fillIn` helper method
* setup importable `focus` helper method
* setup importable `blur` helper method
* setup importable `waitUntil` helper method
* setup importable `waitFor` helper method

Once implemented, `setupRenderingTest` and `setupAcceptanceTest` will diverge from each other in very few ways.

## 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@rwjblue rwjblue Nov 6, 2017

Choose a reason for hiding this comment

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

When running tests with the old moduleForAcceptance API they will be present (as mentioned in the section below about both API's co-existing), this section is referring to tests using setupAcceptanceTest(...) API and is correct in saying that the global methods will not be present.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
* Implicit promise chaining will no longer be present. If your test needs to
wait for a given promise, it should use `await`.
Copy link
Member

Choose a reason for hiding this comment

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

I think it is worth noting that awaited helpers in acceptance tests will wait for all the same things they currently do.

* The test helpers that are included by a new default ember-cli app will be no
longer needed and will be removed from the new application blueprint. This
includes:
* `tests/helpers/resolver.js`
* `tests/helpers/start-app.js`
* `tests/helpers/destroy-app.js`
* `tests/helpers/module-for-acceptance.js`
Copy link
Member

Choose a reason for hiding this comment

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

Does tests/test-helper.js remain?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kategengler - Yes, though IMHO, it would be better to rename it to tests/index.js. It truly is the entry point to all the tests (and even more obvious now that you have to manually call start() to kick off the tests)...

Copy link

Choose a reason for hiding this comment

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

Many folks use module-for-acceptance to start/stop Mirage servers. So the new blueprint wont include that module helper, but nothing is stopping users from keeping or adding it back, correct?

Copy link
Member

Choose a reason for hiding this comment

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

with the new system you would call something like setupMirage(hooks) and it would handle starting and stopping of the server in the right hooks automatically for you


## 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?
Copy link

Choose a reason for hiding this comment

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

An alternative (or maybe just a suggestion?) would be to make ember-native-dom-helpers into a semi-official addon. Instead of copying the source code into ember-test-helpers, why not start shifting the helper code out of the core and into an addon?

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 question, I will add this to the alternatives section.

ember-test-helpers is an addon 😸 (completely separate from Ember's own source), supports many Ember versions simultaneously, is focused on providing consistent testing experiences for Ember users, is already a dependency of every application that uses ember-qunit or ember-mocha (roughly all projects), and is a better name 😈.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 ember-test-helpers to be THE PLACE where all "official" testing stuff (that is not qunit-specific) lives.

* Make `ember-native-dom-helpers` a default addon (removing the need for DOM interaction helpers proposed here).
Copy link

@ro0gr ro0gr Nov 10, 2017

Choose a reason for hiding this comment

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

Looks like ember-test-helpers with the setupTest, moduleFor*, owner and company is already quite massive.

Keeping DOM helpers implementation as a separate package seems like a win to me from the separation of concerns standpoint.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ember-test-helpers or a new addon that is depended on by (and re-exported from) ember-test-helpers is not exactly important.

Also, I disagree that ember-test-helpers is "too big".The non-legacy portions are actually quite small at 423 lines of code (even including the legacy code its ~ only 1353 loc).