Grand Testing Unification #119

Open
wants to merge 13 commits into
from

Projects

None yet
@rwjblue
Member
rwjblue commented Feb 8, 2016

Rendered

@bcardarella
Contributor

yes please

@rtablada
rtablada commented Feb 8, 2016

@rwjblue Cheers

I do think that getOwner(this) would be best to remain consistent though.

@mmun
Contributor
mmun commented Feb 8, 2016

getOwner(this) does not seem consistent to me at all. The test module is not created by the owner and thus is not owned by it. In fact, the reverse is true. The test module creates the owner. The test module has a reference to the owner and chooses to expose it on this.owner.

@mixonic
Member
mixonic commented Feb 8, 2016

The test module is not created by the owner and thus is not owned by it.

Well put @mmun. I agree.

@courajs courajs and 2 others commented on an outdated diff Feb 8, 2016
text/0000-grand-testing-unification.md
+* `Ember.Application#injectTestHelpers`
+* `Ember.Application#removeTestHelpers`
+* `Ember.Test.registerWaiters`
+* `Ember.Test.unregisterWaiters`
+* `Ember.Test.*`
+* existing `ember-qunit` / `ember-mocha` API's.
+
+It is possible that we can automate some of the migration process, we should continue to investigate migration automation as we progress with implementation.
+
+#### Example Migration of Component Integration Test
+
+This is [an existing test](https://github.com/aptible/dashboard.aptible.com/blob/09c76ef827704c065140fe890ef1a4d24c42be36/tests/integration/components/primitive-select-test.js) for a very simple `{{object-select}}` component:
+
+```js
+import Ember from 'ember';
+import { moduleForIntegration } from 'ember-qunit';
@courajs
courajs Feb 8, 2016

moduleForComponent?

@mmun
mmun Feb 8, 2016 Contributor

moduleForIntegration is correct. There is nothing specific to components about it. It can be used to test template helpers, or interactions between components. Think of it like a micro acceptance test.

On the other hand, unit testing a component can be done with moduleForUnit, though I recommend sticking with integration tests unless you have a specific reason to unit test.

In short, we no longer need a moduleForComponent and keeping it (other than for backwards compatibility) would cause confusion.

@courajs
courajs Feb 8, 2016

I thought this is an existing test using the old way. In any case, it doesn't match with moduleForComponent below on line 390, which I think it was meant to

@mmun
mmun Feb 8, 2016 Contributor

You're totally right. My mistake!

@rwjblue
rwjblue Feb 8, 2016 Member

Ya, overeager find/replace. Fixing....

@HeroicEric HeroicEric commented on the diff Feb 8, 2016
text/0000-grand-testing-unification.md
+These helpers will be available in all testing contexts where DOM helpers are applicable (acceptance and component integration tests). All DOM related helpers will use normal DOM API's, and will avoid using jQuery unless required.
+
+- `click` - Async helper to click on the specified selector.
+- `triggerKeyEvent` - Async helper to trigger a `KeyEvent` on the specified selector.
+- `triggerEvent` - Async helper to trigger an event on the specified selector.
+- `fillIn` - Async helper to enter text on the specified selector.
+- `find` - Sync helper to return an element for a given selector (via `document.querySelector`).
+- `findAll` - Sync helper to return a list of elements matching the given selector (via `document.querySelectorAll`).
+- `element` - Property to access the raw element at the root of the DOM for the given test.
+
+#### Rendering/Integration Helpers
+
+These helpers will be available in testing contexts that allow rendering individual template snippets (integration test):
+
+- `render` - Async helper to render a given handlebars template.
+- `clearRender` - Async helper to clear a previously rendered template.
@HeroicEric
HeroicEric Feb 8, 2016

I'm curious about what this would be used for

@mmun
mmun Feb 8, 2016 Contributor

It's used for ensuring that your component tears down correctly (e.g. removes any jQuery handlers it added). Previously, you'd have to wrap your component in an {{#if isActive}} block and toggle isActive to test the same thing.

@rwjblue
rwjblue Feb 8, 2016 Member

@HeroicEric - If you need/want to test custom behaviors in willDestroyElement, you would either have to wrap in an {{if}} like @mmun said, or use this.clearRender() (which forces the rendered template to be torn down).

@martndemus
martndemus Feb 8, 2016

Will this also help clearing before calling this.render a second time?

@mmun
mmun Feb 8, 2016 Contributor

Calling this.render a second time should call clearRender if it hasn't already been called manually.

@rwjblue
rwjblue Feb 8, 2016 Member

@martndemus - Yes, but I would suggest that you should use a different test instead of rendering twice most of the time...

Also, @mmun is absolutely correct, each call to this.render(....) should call this.clearRender() if it had been previously rendered.

@stefanpenner stefanpenner commented on the diff Feb 8, 2016
text/0000-grand-testing-unification.md
+
+# Motivation
+
+Usage of component integration style tests is becoming more and more common, but these tests still 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, but forcing users to manually manage all 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 and improved in 1.2.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 constructs, one for each type of test, that all share the same basic structure and helper system. This new system will be implemented in the `ember-test-helpers` library so that it can iterate faster while supporting multiple Ember versions independently.
+
+This proposal will implement a few core concepts to address these issues:
+
+- Usage of `async`/`await` semantics
@stefanpenner
stefanpenner Feb 8, 2016 Member

related tracking issue for async/await by default ember-cli/ember-cli#3529

@kategengler
Member

πŸ‘

My only thought/concern is around https://github.com/rwjblue/rfcs/blob/42/text/0000-grand-testing-unification.md#registering-custom-waiters

I've seen apps where waiters are registered in code (though it never feels right, but often does seems to be the easiest way to get at the conditions that matter for waiting), and the tests are unaware of the custom waiters. liquid-fire does that here https://github.com/ember-animation/liquid-fire/blob/master/addon/transition-map.js#L106 In the case of addons registering waiters, it does seem nice that each test/app doesn't need to know to register the waiter.

@rwjblue rwjblue Fix over eager find/replace. d5a27f0
@tim-evans

For some references to code in the wild, I'm using the following code for testing:
https://gist.github.com/tim-evans/5982357f7df5c0c045dd, which follows this RFC in some places.

@davewasmer
Contributor

@kategengler this RFC for adding something like run.callback() might be relevant as well (the addon spike uses registerWaiter internally)

@rwjblue
Member
rwjblue commented Feb 8, 2016

@katie:

I've seen apps where waiters are registered in code (though it never feels right, but often does seems to be the easiest way to get at the conditions that matter for waiting), and the tests are unaware of the custom waiters.

Yes, I have seen this as well, but I generally feel that modifying your app code to registerWaiters itself is not good.

I could easily see adding a mechanism for our test setup code to invoke a method on each addon so that the addon could do some custom setup. This would allow them to register custom helpers, waiters, and QUnit assertions. However, I also really like knowing where these things are coming from by registering them in the shared test/helpers/module-for-*.js file.


I will add an "unanswered question" for this, so we do not lose track of this concern.

@rwjblue rwjblue Add unanswered question about API for auto-registering from addons. 30ec7b5
@Gaurav0
Gaurav0 commented Feb 8, 2016

Wow. I just have to say, this is going to mean a lot of upgrading.

First strongly consider (and add to alternatives) adding async helpers to unit/integration tests but not removing anything.

I would strongly suggest:

  1. Do not do this until Ember is addonized. Once blueprints are in the ember package things will be easier.
  2. Make a plan to introduce all of the new features incrementally. Do not make this a "big bang" in a single minor version of Ember / ember-cli.
  3. Ensure that the parts requiring changes to ember-cli, ember-qunit, and ember-mocha are introduced in a way that allows all versions of Ember 1.12 - 3.0+ are forward and backward compatible.
  4. I still think the regsiterWaiter API is too low level. Look at the ember.run.callback PR for ideas on how to improve
  5. Don't wait until 3.0 is almost here to start deprecating stuff. Make a plan to deprecate features 2 to 3 minor releases after their replacements have landed.
  6. Document, document, document. And then document some more.
@Gaurav0
Gaurav0 commented Feb 8, 2016

@stefanpenner While that may be true as far as enabling these features goes, I don't see how this is possible (especially without heavy use of reopen) without modifying ember, ember-qunit, and ember-mocha. ember-cli may not require heavy modification if ember is addonized first.

@stefanpenner
Member

First strongly consider (and add to alternatives) adding async helpers to unit/integration tests but not removing anything.

the current model is a pretty big hazard, but should be supported until 3.0. Transitioning new apps and users away seems prudent. Post 3.0 we can swap, extracting the current to an add-on etc.

You raise a good point though (i think), and that is we should ensure it is possible for both to run in the same code-base at the same time, to ease upgrades.

@stefanpenner
Member

ember-cli may not require heavy modification if ember is addonized first.

This work is basically done (pending using @trabus's new test infrustrature + rebase), we can safely assume it will land before.

@kategengler
Member

@rwjblue Somewhere for addons to hook in would alleviate part of my concern. I agree that modifying app code to incorporate registerWaiter is not good but I haven't seen good alternatives for when you need to get at state inside of the app to know whether to wait. I need to look for concrete examples of when I've encountered it (I may no longer have access to those apps), but if I remember correctly it comes up a lot with custom data solutions and with animations.

@davewasmer Thanks for the pointer to the RFC, I do think there are uses for registerWaiter currently that aren't covered by that or the addon, though.

@backspace

One example is PouchDB-backed Ember applications, which can’t rely on monitoring Ajax calls to know that operations are complete. ember-cli-test-model-waiter has helped me in that sense, but I still have weird intermittent test failures that are probably related to not having proper ordering. I have in the past used a custom helper that waits on a promise set in a controller but it makes me feel dirty 😁

@rwjblue
Member
rwjblue commented Feb 9, 2016

@Gaurav0:

First strongly consider (and add to alternatives) adding async helpers to unit/integration tests but not removing anything.

Nothing is proposed to be removed in this RFC. As stated in the RFC, the plan is to implement everything as new additive API that can be consumed and does not propose changing any existing API's.

Do not do this until Ember is addonized. Once blueprints are in the ember package things will be easier.

The changes here are not related to Ember version, and will not live in the Ember repo.

Make a plan to introduce all of the new features incrementally. Do not make this a "big bang" in a single minor version of Ember / ember-cli.

As I state above and in the RFC, these changes will be additive. When this RFC is approved/merged we will begin rolling out the new API's proposed here.

Ensure that the parts requiring changes to ember-cli, ember-qunit, and ember-mocha are introduced in a way that allows all versions of Ember 1.12 - 3.0+ are forward and backward compatible.

The reason this will be implemented in a separate library instead of Ember itself is so that we can implement exactly this sort of cross version support. As mentioned in the unanswered questions section, the lowest Ember version that would be supported has not been determined, but I would prefer 1.12.

I still think the regsiterWaiter API is too low level. Look at the ember.run.callback PR for ideas on how to improve.

I will await a decision from #115 before embedding those changes here, however it should be very straightforward to implement whatever solution is decided there.

Don't wait until 3.0 is almost here to start deprecating stuff. Make a plan to deprecate features 2 to 3 minor releases after their replacements have landed.

As stated in the "Migration Plan" section of this RFC, the deprecation of the existing testing infrastructure will be completely based on feedback of the new system. If things are going well, we could start deprecating existing features within a minor version or two of when these changes land. I will update that section to put a clearer timeline there.

Document, document, document. And then document some more.

Indeed.

@rwjblue
Member
rwjblue commented Feb 9, 2016

@stefanpenner:

You raise a good point though (i think), and that is we should ensure it is possible for both to run in the same code-base at the same time, to ease upgrades.

All existing API's will remain unchanged and will be able to run alongside and independent of the new proposed API's. It will absolutely take time to upgrade (even if my hopes of some automation assistance come to fruition), and we need to ensure that existing test suites continue to work.

@Gaurav0
Gaurav0 commented Feb 9, 2016

@rwjblue

Nothing is proposed to be removed in this RFC. As stated in the RFC, the plan is to implement everything as new additive API that can be consumed and does not propose changing any existing API's.

Not true.

From the RFC

Based on feedback received from the community on the usability of the new structure proposed here, we will deprecate usage of existing Ember API's:

Ember.Test.registerHelper
Ember.Test.registerAsyncHelper
Ember.Application#setupForTesting
Ember.Test.unregisterHelper
Ember.Test.onInjectHelpers
Ember.Application#injectTestHelpers
Ember.Application#removeTestHelpers
Ember.Test.registerWaiters
Ember.Test.unregisterWaiters
Ember.Test.*
existing ember-qunit / ember-mocha API's.

I suggested, as an alternative, only adding unit/integration async test helpers, not unifying them, and not deprecating any of the above mentioned APIs.

The changes here are not related to Ember version, and will not live in the Ember repo.

My understanding from your and @stefanpenner 's previous statements is that all of the blueprints are planned to be moved from the ember-cli repo to the ember repo as part of addonizing the ember repo, so that they can be correct according to the ember version being used.

@rwjblue
Member
rwjblue commented Feb 9, 2016

@Gaurav0:

Nothing is proposed to be removed in this RFC. As stated in the RFC, the plan is to implement everything as new additive API that can be consumed and does not propose changing any existing API's.

Not true.

Nowhere in the quoted section do I speak of "removing" anything. I speak of deprecating once we are generally happy with the new solution.

I suggested, as an alternative, only adding unit/integration async test helpers, not unifying them, and not deprecating any of the above mentioned APIs.

I will add that to the alternatives section. However, continuing down the path of wall papering over large problems in our infrastructure seems very bad to me.

rwjblue added some commits Feb 9, 2016
@rwjblue rwjblue Make co-existence clear. 0050651
@rwjblue rwjblue Add additional alternative: more wall-papering please. a914078
@rwjblue
Member
rwjblue commented Feb 9, 2016

@kategengler:

liquid-fire does that here https://github.com/ember-animation/liquid-fire/blob/master/addon/transition-map.js#L106

This waiter could be written as such (outside of app code):

// addon-test-support/waiters/running-transitions.js

import { testWaiter } from 'ember-test-helpers';

export default testWaiter(function() {
  let transitionMap = this.owner.lookup('service:transition-map');

  return transitionMap.runningTransitions() === 0;
});

This is roughly the same implementation that exists in liquid-fire today, but as of this moment it would still require manual registration (in the shared tests/helpers/module-for-*.js files). I added a section to "unanswered questions" about either a hook to allow addons to do this work themselves, or automatically registering helpers and waiters across the board (similar to what we do with initializers). I will continue to think/work on that aspect of this RFC...

@kategengler
Member

@rwjblue I think its a scenario with two gross options -- do you let your app know about your testing framework or do you let your acceptance tests know about the internals of your app? If that way (looking up + reaching in) is generally accepted as kosher and with a hook for addons, I think my concerns are allayed.

@rwjblue
Member
rwjblue commented Feb 9, 2016

@kategengler:

I think its a scenario with two gross options -- do you let your app know about your testing framework or do you let your acceptance tests know about the internals of your app?

Yeah, I agree. In this case (liquid-fire), I believe that the test waiter I wrote is just another consumer of a normal public API (the transitionMap.runningTransactions() method). It also has the benefit of not increasing production app size for the benefit of tests.

Regardless, I think we are on the same page. I will try to come up with a nice API for the hook mechanism...

@topaxi
topaxi commented Feb 9, 2016

I love his, we use async/await pretty heavily in our apps and our tests. One thing I learnt was that the transpiled output from regenerator was not really welcomed by other devs, we since switched to the asyncToGenerator transform which matches async/await way better.
In other words, debugging async/await might not be very pleasant in non-generator browsers (Edge 12, any IE, Chrome 38 and earlier, any Safari and iOS, PhantomJS and Android 5.0 and earlier).

@stefanpenner
Member

@topaxi i feel its often more debuggable then the callback variant :P after all it is just a big switch statement...

@trabus
trabus commented Feb 9, 2016

I love this, async/await would be wonderful.

@rwjblue One thing I was wondering if you could address, how would concurrent tests fit into this scenario? Is that on the horizon at all, or do we have to wait for the testing libraries to provide that functionality?

@acorncom
Member
acorncom commented Feb 9, 2016

I like the way things look here (and the newer syntax), so appreciate your work on thinking through this @rwjblue

Regarding deprecations, with the new LTS release approach, it seems like we're generally aiming for deprecations to occur for at least four releases. For these types of sweeping changes, could we extend that in some way? Have the deprecations last for 8 releases so that folks moving from one LTS release to another have a longer time frame to make the shift? That may be too long ...

@rwjblue
Member
rwjblue commented Feb 9, 2016

@trabus:

One thing I was wondering if you could address, how would concurrent tests fit into this scenario? Is that on the horizon at all, or do we have to wait for the testing libraries to provide that functionality?

This RFC does not intend to completely solve that problem, however it does make things a bit better for parallel tests by removing the usage of the global acceptance test helpers.

@rwjblue
Member
rwjblue commented Feb 9, 2016

@acorncom:

Regarding deprecations, with the new LTS release approach, it seems like we're generally aiming for deprecations to occur for at least four releases. For these types of sweeping changes, could we extend that in some way?

The current acceptance testing system will have to remain deprecated throughout the life of Ember 2.x (since it is distributed with Ember itself), which is likely to be quite a while still and it is quite feasible to let them live on and be used even into 3.0 if we see that there is still need for them. The current form of integration and unit tests are already distributed as an independent library and the application developer controls the version that is used. So they can continue to author tests just as they are now as long as they want. As I mention in the RFC (and recently tweaked the language of) I intend to support both the current implementation and the new implementation in the same library, so both types of testing would continue getting bug fixes as needed. We will do a major version bump (removing the existing semantics) only after we believe that the vast majority of users are on the newer system proposed here (aka a long time).

@rwjblue rwjblue referenced this pull request in teddyzeenny/ember-mocha-adapter Feb 11, 2016
Closed

Migrate into ember-mocha? #35

@Turbo87 Turbo87 and 1 other commented on an outdated diff Feb 11, 2016
text/0000-grand-testing-unification.md
+ assert.equal(newValue, 'two');
+ });
+
+ this.set('listing', Ember.A(['one', 'two', '3', '4']));
+
+ await this.render(hbs`{{primitive-select update=(action "checkValue") items=listing}}`);
+
+ await this.fillIn('select', 'two');
+});
+```
+
+# Pedagogy
+
+The main changes that this RFC makes to pedagogy is to massively simplify the set of testing special cases that consumers have to manage mentally while developing an application. The divide between acceptance tests and unit/integration tests largely vanishes, and we begin leveraging language features where we previously implemented a custom DSL.
+
+It is extremely important that as these changes land in the various libararies, we ensure the testing guides listed on guides.emberjs.com are kept up to date. A rewrite of large sections of the existing guide are likely needed, but this rewrite should result in a massive improvement due to the conceptual overhead that is being removed.
@Turbo87
Turbo87 Feb 11, 2016 Member

libararies -> libraries ;)

@rwjblue
rwjblue Feb 11, 2016 Member

Good catch, updated...

@rwjblue rwjblue What the heck is a libararies? 87a4765
@workmanw
Contributor

I'm not completely sure if this fits into this RFC or not, so I'm going to bring it up.

How does this impact testing ember-data based classes? Specifically Adapters, Serializers, Transforms, and Models. For better or worse we have a decent amount of custom transforms, serializers and adapters we test. Also model based CPs / functions. Additionally we like to test implicit assumptions we've made about ember-data's behavior so we can detect potential regressions when upgrading ember-data.

Most of the time we consider these types of tests to be integration tests, especially since some parts of ember-data API is not easily mockable for testing, IE snapshots. AFAIK moduleForModel is really meant for unit test work. Currently we have our own half-baked test suite for integration testing this layer of our app. Most of the stuff we need to do is mock the adapter, then interact with store and models. This all feels very similar to how moduleForIntegration works, but based on the ember-data world.

I guess my question is how do you see that fitting into this RFC?

rwjblue added some commits Feb 11, 2016
@rwjblue rwjblue Remove bizarre character. e5846c3
@rwjblue rwjblue Add auto-resolving of helpers and waiters. 48aee6b
@rwjblue rwjblue Add general test configuration hooks. 66f4cf3
@rwjblue rwjblue Remove answered "unanswered" question... 9515ae8
@rwjblue
Member
rwjblue commented Feb 11, 2016

I have just updated this RFC in order to address a few of the concerns mentioned in various comments. The main changes are:

  • Add auto-registration of application helpers and waiters. Read along here for details.
  • Add general hooks that can be used by applications and addons to prefer common setup tasks from specific hooks (beforeSuite, beforeEach, afterEach). Read along here for details.
@rwjblue
Member
rwjblue commented Feb 11, 2016

@kategengler - I believe that the "General Hooks" section should directly address the concern of addons providing helpers/waiters.

@rwjblue
Member
rwjblue commented Feb 11, 2016

@workmanw

Thanks for chiming in, and chatting with me in Slack about this! The takeaway from our conversation is that moduleForIntegration will absolutely be the way to handle these sorts of tests, and that specific addons will be able to provide custom helpers (see "General Hooks" section I just added). I definitely envision ember-data living in this world, providing its own set of special/custom helpers to make testing serializers/adapters/models much easier.

As we progress through this RFC process and begin writing updated documentation and guides we will see what kinds of helpers make sense in the models section, and work with the Ember Data team to get them added.

I think the key here in this RFC, is that we are providing an infrastructure so that "special one-off" integrations (like ember-data is with today's ember-test-helpers) will not be needed, and all addons can hook in and provide helpers/waiters/setup/etc that is needed...

@workmanw
Contributor

@rwjblue πŸ‘ Thanks for taking the time to address that! Cheers 🍻.

@rwjblue rwjblue Add `testInfo` property information. 1ede9e3
@rwjblue
Member
rwjblue commented Feb 12, 2016

Just updated the RFC adding a section on adding a testInfo property to the test context. This will allow helpers to detect which type of test they are in use in, and more...

Read along here for more details...

@lukemelia
Member

πŸ† These changes would be most welcome. Today's inconsistency between test types is frustrating.

@rwjblue
Member
rwjblue commented Feb 12, 2016

@lukemelia - Thanks for reviewing!

@eccegordo

Huzzah! I have avoided writing ember tests for so long. This may win me over.

Couple quick comments:

aesthetics

I actually don't mind the aesthetics of

await this.click(...)
await this.fillIn(...)
assert.equal(this.find(...), ...)

Very clean!

Anything that avoids the gnarly andThen is a major win for readability of tests. Promises are just not very ergonomic looking.

grammar

import { testWaiter } from 'ember-test-helpers';
import { hasPendingTransactions } from 'app-name/services/transactions';

export default testWaiter(function() {
  return hasPendingTransactions();
});
After helpers are invoked, the system continues to "wait" until hasPendingTransactions returns true.

That sentence reads really weird to me. Reading for grammatical tense I would expect

After helpers are invoked, the system continues to "wait" until hasPendingTransactions returns FALSE.

Or even better simple past tense example to communicate the timing/sequence

After helpers are invoked, the system continues to "wait" until transactionsFinished returns true.

@eccegordo

@Gaurav0

Surely there is a meaningful distinction to be made between removing and deprecating some api.
Deprecation is a hint that it is subject to removal "in the future". But not actually removing until semver change clearly signals the breaking change.

I am with @rwjblue on "not wall papering over" the infrastructure problems.

Minor confession I have a not insignificant ember app in production for over three years but basically very few meaningful tests on the ember side because the testing story has been in so much churn, and frankly has sucked or more likely rather I suck at writing tests.

But time marches on and several all nighters and lost weekends diligently doing code reviews and patches have been the cost of this missing test infrastructure. Basically a very labor intensive process to make sure it "continues to work". And all the while moving from

  • Ember Rails --> Ember AppKit Rails --> Ember CLI 0.15 ....1.13.15
  • Ember 1.0 beta --> Ember 1.0 --> Ember 1.1 --> 1.2 --> 1.3 --> 1.4 --> ... 1.10 --> 1.11 --> 1.12 --> 1.13 --> 2.0 --> 2.1 --> 2.2
  • Ember Data (don't even ask) 0.14 --> 1.0.beta1 -> every version --> 2.2.0

This RFC is a huge step forward and I would rather not let over caution prevent a better future.

Dans ses Γ©crits, un sage Italien Dit que le mieux est l'ennemi du bien.
(In his writings, a wise Italian says that the best is the enemy of good.)
~ Voltaire

@pavloo pavloo commented on the diff Mar 16, 2016
text/0000-grand-testing-unification.md
+ await this.click('.submit');
+});
+```
+
+- Due to the usage of `async` / `await` no special casing of sync vs async helpers is needed.
+- The helper shares a context with the test, so it can access the other helpers or properties as needed. This is somewhat dependent on testing framework (some pass arguments for context while others like QUnit assume an implicit `this` context is provided by the framework).
+
+#### Registering Custom Waiters
+
+When the acceptance test helpers were initially released the system waited on three main things:
+
+- run loop queues to be flushed
+- all route async hooks to be completed (`beforeModel`, `model`, `afterModel`, etc)
+- pending AJAX requests
+
+It soon became clear that we needed to expose a way for users to influence what is waited on after each async interaction (i.e. `click`). Thus the `Ember.Test.registerWaiters` and `Ember.Test.unregisterWaiters` API's were added. Unfortunately, these API's were global and users were forced through annoying hoops to use them in a modules world.
@pavloo
pavloo Mar 16, 2016

Would be great if waiters could be attached to a certain async helper call instead of registering them globally. Possible public API (based on the example below):

import transactionWaiter from '../waiters/pending-transactions';
...
test('stuff happens', async function(assert) {
  await this.click('.foo', transactionWaiter);
  // all pending transactions are completed here...
  await this.click('.bar');
  // second click doesn't care about transactions
});
@pavloo
pavloo Mar 16, 2016

well looks like you can just simulate this with register/unregister

@blimmer
blimmer commented Mar 30, 2016

Overall, I really like the unification of integration and acceptance testing proposed in this RFC. I agree that it's definitely confusing to switch contexts and use different semantics in either scenario.

Automated Transforms?

I'm trying to think through is whether or not it would be possible to provide an automated way for folks to transition their acceptance tests over to the new await style. With the previous style, the andThen obscured other promises, so there would be no easy way to break this down with an ember-watson transform that would take something like:

visit('/foos');
click('button');
andThen(function() {
  expect(currentRouteName()).to.equal('/success');
});

and turn it into

await this.visit('/foos');
await this.click('button');

expect(currentRouteName()).to.equal('/success');

Because of all the additional waiting that the andThen provided. For folks who have thousands of acceptance tests (like us), having to touch every single test to get onto the new standard is a really hard pill to swallow (and you've noted that in the drawbacks).

Waiting for Promises created in non-singletons (e.g. Components)

In many cases, I'll try to load all non-critical data in promises outside of the model
hooks. I often find myself doing something like this:

// app/routes/foos.js

model() {
  return this.store.findAll('foo');
}
// app/templates/foos.hbs

{{#each model as |foo|}}
  ...
{{/each}}

... below "the fold" ...

{{slow-stuff}}
// app/components/slow-stuff.js

store: service(),
slowData: computed(function() {
  return this.get('store').findAll('slow');
}),
// app/templates/components/slow-stuff.hbs

{{#if slowData.pending}}
  ... spinner ...
{{else}}
  {{slowData}}
{{/if}}

This RFC makes this behavior testable (since we can now assert before the Component Promise is resolved), but it's not clear to me how I would register a waiter to assert the behavior after the slowData promise resolved. Would I need to somehow export a function from the Component definition that knows about the computed promise?

@Serabe
Serabe commented Aug 5, 2016

I would remove the usage of some of the helpers if the selector matches more than one element. For example, fillIn currently only changes the value of the first match and leave the rest untouched. I propose to deprecate that behaviour and raise an error.

This deprecation would apply to all DOM helpers except findAll, since the user expects a collection back, and element, since it is not querying anything.

@martndemus

What's holding this bad boy from moving into FCP and implementation?

@mixonic
Member
mixonic commented Nov 21, 2016

I believe the RFC must be reconciled with the Dan's module unification RFC and with Tom's ES modules RFC. Additionally it may be prudent to consider some of the APIs in light of efforts like Igniter (the refactor of Ember's runloop to use the microtask queue).

Practically, the world has been on pause while Glimmer2 was the focus of most effort. @rwjblue can perhaps chime in with more, but I expect the blockers here can and should be a focus during the Dec F2F.

@machty
Member
machty commented Dec 27, 2016 edited

Been discussing with @rwjblue but wanted to share a potential tweak to test helper semantics that I think will solve a lot of tricky issues pertaining to route loading substates and other intermediate/transient states that occur while test waiters are still unsettled.

Let's say you have the following:

await this.click('.some-btn');
await this.click('.another-btn');
let $sel = await find('.banner');
assert.equal($sel.text(), "wat");

I don't think the semantics of the above are 100% spelled in the RFC, but most would assume the behavior is: 1) try clicking .some-btn immediately, pause the async fn, and don't resume until all the test waiters settle, and then 2) try clicking .another-btn immediately, and again wait for its effects to settle as reported by all registered test waiters, and then 3) try finding .banner immediately, etc.

In all cases, if the selector (e.g. .some-btn) isn't found right away (synchronously), this throws a failed assertion.

These are the somewhat classic semantics that most people have come to expect from ember-testing, but with these semantics, it's still really hard to test:

  1. Route loading substates or any transient states that occur while test waiters are still unsettled
  2. Code with long / looping timers

There is a subtle tweak that has been proposed that (I believe) maintains backwards compatibility and should for the most part remain in line with people's mental models. Given the same code above:

  1. While test waiters are unsettled, try repeatedly to find the selector (e.g. .some-btn).
  2. Once you find the selector, "click" it.
  3. Resume the async testing function immediately, without waiting for the effects of the click to "settle" (i.e. resume the function before test waiters have necessarily settled)
  4. If test waiters settle before the selector can be found, throw a "Selector Not Found" failed assertion.

I'd call this the "retry-until-settled" semantics. If this seems weird/unusual, keep in mind that if you're structuring tests using the async fn approach proposed in the RFC, it doesn't actually change anything about the behavior of the test, aside from the following benefits:

1. Loading substates are now testable

For instance if you use loading substates in your app, and your loading template has <h1 class="loading-banner">Loading...</h1>, you can now test that loading substate as follows:

await this.click('a.some-link-to-slow-route');
let $loadingBanner = await find('.loading-banner');
assert.equal($loadingBanner.text(), "Loading...");

let $slowRouteBanner = await find('.slow-route-banner');
// this will resume when the slow route finishes loading

Previously, there was no easy way to test loading substates because ember-testing wouldn't run your andThen() or wait() callbacks while test waiters were still running, but with the "retry-until-settled" approach, your tests resume more eagerly, once the selector appears in the DOM.

2. Tests are more immune to timers

Since ember-testing's internal waiters block on unfired Ember.run.later timers, this can lead to frozen tests that eventually time out due to a) long timers or b) polling loops, oftentimes unrelated to the feature under test.

With "retry-until-settled", you can test features even if some background outstanding timer hasn't yet fired.

NOTE: this is only a partial solution to the trickiness surrounding testing timers. This approach does make things easier / faster when tests pass, but it also means that if a selector fails to match, the test will only fail when test waiters settle (or the test case times out). In other words, there's still room for testing abstractions brought to you by ember-lifeline, or by using smaller timer values or breaking timer loops when testing. But keep in mind that this downside is also present in present ember-testing semantics.

timeout option

I think we should also consider an option for find() (and all the helpers that use it) to wait beyond test waiter settlement for an element with matching criteria to appear. This is useful when writing a custom waiter is difficult, annoying, or doesn't really make sense (e.g. what does it mean for a WebSocket connection to "settle").

One possible API might look like:

await this.click('a.some-link', { timeout: 5000 });

The timeout option implies: "if this selector hasn't matched by the time test waiters have settled, wait an additional 5 seconds for it to show up before failing". This essentially opts into "Capybara" semantics, which shouldn't be the default since it slows the TDD cycle (and slows down the test suite when tests are failing).

Admittedly, this option is not as crucial as "retry-until-settled" semantics, but I'd at least want it to be possible to implement such a helper myself without being boxed in by hard-wired waiter logic.

@courajs
courajs commented Dec 28, 2016

Can "retry-until-settled" be made reliable for testing loading routes? It leaves me feeling a little nervous that the runtime might not "catch" the loader before the model resolves.
Maybe it will be reliable because it'll check on every re-render, but it might inspire more confidence to have an explicit test helper for loading routes.

@machty
Member
machty commented Dec 28, 2016

@courajs I don't know what the convention is, or whether there is even one, but I would think most people testing something timing-dependent things like loading routes are already using testing tools to control the resolution timing of model hook promises (perhaps ember-cli-mirage does this?). Something like the following:

let resolveModel = stubModelHooks();
await this.click('a.some-link-to-slow-route');
let $loadingBanner = await find('.loading-banner');
assert.equal($loadingBanner.text(), "Loading...");

resolveModel({ name: "fake model" });

let $slowRouteBanner = await find('.slow-route-banner');

This pattern seems pretty robust against timing dependencies (or at least reduces them to the same robustness as code that resumes after test waiters settle).

That said I do think we might want to consider using something like Mutation Observers to "kick" a paused retrying finder so that by default Ember is catching as many intermediate states as possible, but generally speaking I think a stubbing/mocking controller model resolution solution makes the most sense here.

@jgwhite
jgwhite commented Dec 28, 2016

@machty iirc Capybara uses something much like retry-until-settled and it seems to work well. Perhaps worth discussing: they ended up adding helpers of the form assert page.has_no_content?("wat") as subtly distinct from assert !page.has_content?("wat") to deal with the case where you're expecting "wat" to disappear and the latter form will settle early and give a false negative.

@machty
Member
machty commented Dec 28, 2016 edited

@jgwhite FWIW I've been spiking out some of the APIs for this RFC in ember-concurrency land here

This is the API I had in mind for what you're describing:

https://github.com/machty/ember-concurrency/blob/4099e09/tests/unit/test-utilities-test.js#L112-L119

Basically, find() would have a count option that defaults to 1 and asserts if the selector finds more or less than the count provided. If you expect an element to disappear, use count: 0. I think this API feels mostly nice, and encourages a fail-early succinctness (though I wonder if a better default when count is unspecified would be to match any number of elements greater than 1, since I can imagine it being hard/impossible/brittle in some testing environments to know up front how many of a certain element might appear, e.g. if you're testing some network that doesn't always return the same number of items).

RE Capybara: they only have that awkward unfortunate distinction for their basic vanilla set of assertions, but the problem goes away when using RSpec. In our case, since all find()ers are async and need to be awaited, we avoid the problem of two syntaxes where one fails synchronously.

Also, to be clear, these insights were inspired by my experiences with Capybara, but the big difference between what I'm proposing and Capybara is that unlike Capybara, we have an established notion of settledness that we leverage to make our assertions/finders fail ASAP. Specifically, the default behavior of the find() helper I'm proposing is that once test waiters have settled (timers have elapsed, routes have full transitioned, etc) an assertion will be thrown if find() still hasn't found a match. Capybara can't do this (or I'm unaware of it) because there's no built-in concept of test waiters. This means that Capybara assertions fail only when some timeout is reached, which makes for a poor/slow experience when doing TDD since you have to wait for that timeout on every failed test assertion. (Also, I found Capybara to be inconsistent with which test helpers / assertions had the waiting behavior; finding elements would wait, but attempting to click them wouldn't, etc; I think we can do better)

@jgwhite
jgwhite commented Dec 28, 2016

@machty find with a count sounds awesome to me and might help catch other classes of subtle non-async testing bugs. Totally agree with your thoughts on capybara.

@courajs
courajs commented Dec 28, 2016

Would find with count: 0 resume the test function immediately as soon as 0 instances of the selector are present? How would I test that, once the transition to a new route has finished, a selector isn't present on the new page?
Using find to force settling seems a little awkward, and the count: 0 means that an assertion core to the test is implicit in find:

await this.click('.delete-all-users');
await this.click('a.nav-to-user-list');
await this.find('.exists-on-user-list');
await this.find('.user', { count: 0 });
@machty
Member
machty commented Dec 28, 2016

@courajs I think you've identified the fundamental tradeoff with this API, in that there are definitely still some awkward cases where you do want the settling behavior. I think in this case you have two options:

  1. await this.wait() (or perhaps it'll be renamed to await this.settle() to be less awkward/redundant) before finding/asserting
  2. Make a positive assertion about the post transition state before making negative (count: 0) assertions.

Option 2 isn't unique to what I'm proposing; there's been times in the past with classic test waiter behavior where I click()ed a button that essentially was no-op-ing, and getting false passing assertions (e.g. "after submitting this form, I expect NOT to see an error banner"). In these cases, a sprinkling of a positive assertion got me back on the right path.

Maybe there's a way we can improve this testing story even further; I just don't see any alternative test-waiter-y API that doesn't suffer from the major issues of making intermediate states untestable (same goes w WebSockets).

an assertion core to the test is implicit in find

I might be misunderstanding you but I actually like that within find() is an implicit assertion. It often entirely removes the need for a separate line of test code to ensure that $sel.length === 1 and feels SQL-y (in a good way) where you give the framework more info up front to be smart about things. And you can always do additional asserts against the selector if need be.

Either way, I think it might be a good idea to publish an addon that let's people try out some of these ideas and see how they actually feel in production apps.

@tchak
Member
tchak commented Dec 30, 2016

I have one grief already present in current test system and which will amplify with this new proposal. The fact that we will rely on this makes it impossible to write test(() => {}). I know this is basically an aesthetic issue, but could we have some way to access test context through a parameter for these who prefer a more functional style?

test('should be ok', ({ assert, context, helpers }) => {
  context.owner.lookup();
  context.get();
  helpers.find();
  assert.ok();
});
@rwjblue
Member
rwjblue commented Dec 30, 2016

@tchak - That is basically a qunit/mocha issue. I know on the QUnit side @martndemus opened an issue a while back about this (I believe the suggestion was to pass the context as a second argument or something?).

@tchak
Member
tchak commented Dec 30, 2016 edited

@rwjblue I kinda like my idea with hash destructing. But as I said, this is mostly an aesthetic issue. It will be very easy to come up with an addon to make it work the way I want it. So not a big deal.

@bendemboski

I have a proposal that isn't exactly related to all of this, but might end up being, so I want to mention it here.

One small thorn in my side that has come up a few times is the fact that AFAIK ember-test-helpers doesn't provide support for tests whose subject doesn't have a registered factory. So if my test subject is an object without a registered factory, or is something other than an object, like a mixin (and my test is going to subclass directly), I have 2 options that I'm aware of:

  1. Write a test module that doesn't use any of the moduleFor variants, so just a QUnit module without all the isolated container goodness of ember-test-helpers, and then write a bunch of custom code duplicating moduleFor functionality.
  2. Pick some random factory as the subject of my test.

I usually opt for number 2:

moduleFor('router:main', 'my mixin or whatever', function (assert) {
});

It doesn't look like it would be that difficult to factor the subject logic out of AbstractTestModule and then create a fourth layer in the module-class hierarchy (we might want a better name):

class AbstractTestModule {
  // as-is today
}

class IsolatedContainerTestModule extends AbstractTestModule {
  // All the code from TestModule except subject-related code
}

class SubjectTestModule extends IsolatedContainerTestModule {
  // identical to TestModule's current functionality
}

class IntegrationOrAcceptanceOrWhateverModule extends SubjectTestModule {
  // as-is today
}

which would allow, for example, a nice way of testing mixins that call into a service via getOwner():

import Ember from 'ember';
import { containerModule } from 'ember-qunit';
import MyCoolMixin from 'my-addon/mixins/cool';

containerModule('my cool mixin', function() {
  needs: [
    'service:something-or-other'
  ]
});

test('it works without a someFunction() override', function(assert) {
  this.register('object:test', Ember.Object.extend(MyCoolMixin));
  let obj = this.container.lookup('object:test');
  Ember.run(() => obj.doSomething());
  let service = this.container.lookup('service:something-or-other')
  assert.equal(service.get('callCount'), 1, 'it called the service');
});

test('it works with a someFunction() override', function(assert) {
  this.register('object:test', Ember.Object.extend(MyCoolMixin, {
    someFunction() {
      return;
    }
  }));
  let obj = this.container.lookup('object:test');
  Ember.run(() => obj.doSomething());
  let service = this.container.lookup('service:something-or-other')
  assert.equal(service.get('callCount'), 1, 'it called the service');
});

That may seem like a long way to go just to avoid including 'router:main',, but I think it makes a lot of sense architecturally.

I've though about working on a PR for this functionality, but that seems silly with the test unification on the horizon, so I wanted to bring this up here to get people's thoughts on it, and see if we want to factor it into the work described here, or if I should just go file an issue in ember-test-helpers, or if anybody has another idea for how to support use cases such as the one I've described.

@rwjblue
Member
rwjblue commented Jan 6, 2017

One small thorn in my side that has come up a few times is the fact that AFAIK ember-test-helpers doesn't provide support for tests whose subject doesn't have a registered factory.

Hmm, I believe it does not attempt to lookup a registered object if you specify integration: true or if you specify a factory method in the options argument (the same one that can contain beforeEach or afterEach). Please open an issue over there for that if this doesn't work.

@bendemboski

@rwjblue I'm not following. If I use moduleFor, I get a TestModule, which sets this.subjectName to be the first argument, and treats this.subjectName as the name of a factory here and here (although I guess the second one won't get triggered without a call to this.subject()). It doesn't look like it's at all expecting to not get a factory name.

Am I really confused here? I'm happy to go file an issue in ember-test-helpers, but it sounds like you're talking about it like it's a bug, and it looks like it was never intended to work that way...

@machty machty referenced this pull request in machty/ember-concurrency Feb 10, 2017
Open

RFC: better approaches for dealing with timers in tests #120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment