Rethink Acceptance Testing #268

rwjblue · 2017-11-06T00:26:06Z

Continues the testing related RFC's (slowly chipping away at #119) by proposing migrating the acceptance testing API "unify" (though I'm not allowed to use that word in the title any more) with the newly landed and implemented ember-qunit API's (proposed in #232).

rendered

simonihmig

Really looking forward to see the testing unification (oups, I said it 😬) move forward! But could you maybe explain the relationship between this and #119? Does it supercede the older RFC, at least with regard to acceptance testing?

simonihmig · 2017-11-06T13:41:37Z

text/0268-acceptance-testing-refactor.md

+});
+```
+
+As you can see, this proposal is unifies on Qunit's nested module syntax following


simonihmig · 2017-11-06T13:43:18Z

text/0268-acceptance-testing-refactor.md

+  * 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))


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?

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

simonihmig · 2017-11-06T13:45:38Z

text/0268-acceptance-testing-refactor.md

+  * 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))
+  * setup a getter for `this.element` which returns the DOM element representing
+    the root element


What would the root element be in the case of an acceptance test? The app's root element?

@simonihmig - Yes. I will update the RFC though.

simonihmig · 2017-11-06T13:57:14Z

text/0268-acceptance-testing-refactor.md

+  * setup a getter for `this.element` which returns the DOM element representing
+    the root element
+  * if `jQuery` is present in the application sets up `this.$` method to run
+    jQuery selectors rooted to `this.element`


Actually, I would like to not do this! Although this is not part of this RFC, I feel the general direction for Ember as regarding to its relation to jQuery should be to get rid of it, IMHO. At least we should try to reduce/eliminate the coupling as much as possible, so it is easy for users to opt out. Which is technically possible today with things like @cibernox's ember-native-dom-helpers and your ember-native-dom-event-dispatcher. While this does not technically increase the coupling maybe, I feel it still opens a trap for users, who might want to use jQuery then because of convenience and because it seems to be actively supported by this API, but later suffer from the pain having to refactor those tests when they chose to move away from it.

When dropping this, they could still wrap this.element in a jQuery/$ call if they really want to use jQuery, but we should not endorse to do so, IMHO!

I had written a very long explanation about how you were wrong, at the end of which I realized you were right...

Suggesting that folks use $(selector, this.element) as needed (and from the codemod) seems absolutely fine to avoid creating a cliff.

simonihmig · 2017-11-06T14:04:08Z

text/0268-acceptance-testing-refactor.md

+helpers are being introduced to help avoid extra noise (e.g. "rightward shift")
+in tests.
+
+This means that users will be able to use either of the following interchangably:


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

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

I updated some verbiage in this section to hopefully clarify things a bit better.

simonihmig · 2017-11-06T14:05:25Z

text/0268-acceptance-testing-refactor.md

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


Really not present, or rather deprecated? So they can co-exist, see the following section.

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.

That makes sense! Maybe this should be stated to prevent misunderstandings like I had?

rwjblue · 2017-11-06T14:23:51Z

But could you maybe explain the relationship between this and #119? Does it supercede the older RFC, at least with regard to acceptance testing?

Yes, basically #119 is too large and too sprawling to actually implement. I started chopping it up into smaller more bite sized RFCs (like #232 and this one) so that we can make incremental progress forward. The main thing left in #119 that still is not addressed by either #232 or this RFC is the auto-discovery of test helpers and hooks (this section).

Turbo87 · 2017-11-06T14:58:19Z

text/0268-acceptance-testing-refactor.md

+* 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


why do we need those methods?

@Turbo87:

The main reason set / setProperties methods exist today, is that sets during testing trigger an assertion if not manually wrapped in a runloop. This is something that we are actively working to remove, but that effort will likely take time (as it essentially requires Ember 3.0 where we can rely on all supported platforms having a microtask queue).

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

thanks for the explanation, but I still don't see in what situations one would use e.g. this.set() in an acceptance test 🤔

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

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

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 ember-test-helpers, but that's quite simple anyways.

NLincoln · 2017-11-06T15:20:27Z

text/0268-acceptance-testing-refactor.md

+
+# Alternatives
+
+* Do nothing?


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?

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

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.

cibernox · 2017-11-06T15:46:31Z

text/0268-acceptance-testing-refactor.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


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

this.click = function(e) { assert.ok(e, 'The event is fired') } await render(hbs`{{my-component action=click}}`)

messing with the helper. It could be fixed by making this.click a readonly property, but it still feels weird.

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?

FYI There is one potential advantage of using await this.click() over await click() in that you could potentially run tests concurrently. The click() variant has no reliable way of figuring out which test just called that helper, while this.click() is bound to the right context.

There is one potential advantage of using await this.click() over await click() in that you could potentially run tests concurrently.

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.

Even if I slightly favour the imported helpers, my point is about having both. I think it should be one or the other.

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?

Sure, I agree that this is definitely a reasonable way to go also. I'll add a fleshed out version of it to alternatives.

Even if I slightly favour the imported helpers, my point is about having both. I think it should be one or the other.

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

Agree. Made the same point here. @rwjblue just added some clarification here, which seems reasonable. I would not see it as a problem to have both, as long as there is one ideomatic way to write your tests, that is consistently used (guides, blueprints, codemods etc.) Was too late here. My comment was refering to Miguel's comment, but Rob already clarified this 😉

Oh, y'all did it now. Just removed the this.foo versions from the RFC. I agree having two different public API's for the same thing is confusing, and reserving a bunch of names on the test context is annoying and error prone for users.

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

No longer publicly expose two systems for the same thing, all in on imports...

rtablada · 2017-11-08T16:12:59Z

@rwjblue I like the idea of moving the functions from ember-qunit to ember-test-helpers.

I would mention that the summary does a good job of describing motivations, but doesn't really give a top level explanation of what changes are being proposed and assumes that contributors are deeply in the know on testing unification and prior RFCs.
I think we should add a sentence of the actual impact on how this moves the needle forward.

Turbo87 · 2017-11-08T16:18:11Z

text/0268-acceptance-testing-refactor.md

+// **** after ****
+import { module, test } from 'qunit';
+import { setupAcceptanceTest } from 'ember-qunit';
+import { visit, fillIn, click } from 'ember-test-helpers';


as mentioned in emberjs/ember-test-helpers#240 we might want to move to @ember/test-helpers for the import paths

Good point @Turbo87, updated in ebfdbe1.

rwjblue · 2017-11-08T16:40:10Z

@rtablada

assumes that contributors are deeply in the know on testing unification and prior RFCs

I was trying to avoid this by cross-link to any prior work that I referred to, did I miss some references?

I think we should add a sentence of the actual impact on how this moves the needle forward.

Sounds good, I've tried to do a better job and pushed those changes in b05ed2c. Mind reviewing?

rtablada · 2017-11-08T16:44:13Z

@rwjblue seems like the main big changes are:

Moving helpers from ember-qunit to ember-test-helpers
Favor async/await
Change to typescript in the dead of night :P

rwjblue · 2017-11-08T16:46:09Z

Change to typescript in the dead of night :P

No one is suggesting this. Certainly not me.

rtablada · 2017-11-08T16:47:09Z

I was kidding. But the declare module syntax may be a bit confusing to people.

raido · 2017-11-16T05:33:56Z

This is awesome. 🎉

rwjblue · 2017-12-01T20:11:12Z

This was discussed at the Ember core team meeting today, and given that no objections / suggestions / tweaks have come up during the final comment period, and it seems that most folks massively in favor of this improvement we are going to land....

FWIW, I have begun implementation and hope to be able to use this in apps soon...

Implement `startApplicationTest` from emberjs/rfcs#268.

As of ember-source@3.0.0 (and ember-data@3.0.0) the testing blueprints are automatically emitting emberjs/rfcs#232 or emberjs/rfcs#268 compatible output. With those, these helpers are no longer used for new apps. Existing apps should only delete these files once they have migrated to the new testing system...

As of Ember 3.0 the new testing APIs introduced in emberjs/rfcs#232 and emberjs/rfcs#268 are enabled and used by default. In these APIs no globals are used, therefore this extra `tests/` override is removed. This specifically removes the following globals from being allowed: * `andThen` * `click` * `currentPath` * `currentRouteName` * `currentURL` * `fillIn` * `find` * `findWithAssert` * `keyEvent` * `pauseTest` * `resumeTest` * `triggerEvent` * `visit` * `wait`

As of ember-source@3.0.0 (and ember-data@3.0.0) the testing blueprints are automatically emitting emberjs/rfcs#232 or emberjs/rfcs#268 compatible output. With those, these helpers are no longer used for new apps. Existing apps should only delete these files once they have migrated to the new testing system...

As of Ember 3.0 the new testing APIs introduced in emberjs/rfcs#232 and emberjs/rfcs#268 are enabled and used by default. In these APIs no globals are used, therefore this extra `tests/` override is removed. This specifically removes the following globals from being allowed: * `andThen` * `click` * `currentPath` * `currentRouteName` * `currentURL` * `fillIn` * `find` * `findWithAssert` * `keyEvent` * `pauseTest` * `resumeTest` * `triggerEvent` * `visit` * `wait`

rwjblue added 3 commits November 5, 2017 19:09

Rethink Acceptance Testing

4ec916b

Tweaks/corrections.

f779b71

Tweak alternative.

3ee1064

rwjblue added the T-testing label Nov 6, 2017

rwjblue self-assigned this Nov 6, 2017

rwjblue mentioned this pull request Nov 6, 2017

Support moduleForAcceptance ember-codemods/ember-qunit-codemod#60

Closed

simonihmig reviewed Nov 6, 2017

View reviewed changes

Turbo87 reviewed Nov 6, 2017

View reviewed changes

rwjblue added 4 commits November 6, 2017 10:13

Fix typo in intro of detailed design.

cdbaecb

Better document the DOM Helpers in the detailed design.

3c57791

Clarify that root element meant the applications root element.

5e6c1bd

Remove this.$ helper.

12f4c7b

NLincoln reviewed Nov 6, 2017

View reviewed changes

rwjblue added 2 commits November 6, 2017 10:21

Clarify the reasoning for both helper types.

4bb6857

Add alternative about leaving ember-native-dom-helpers.

a6950d9

cibernox reviewed Nov 6, 2017

View reviewed changes

rwjblue added 4 commits November 6, 2017 11:56

Remove reference to this.set / this.setProperties / etc.

f7656ea

Remove public API suggestion for DOM interaction methods.

8975fb9

No longer publicly expose two systems for the same thing, all in on imports...

Remove more this.foo references.

7ee7132

Add blurb about removing implicit promise chaining.

1209ed3

rwjblue mentioned this pull request Nov 8, 2017

Use Application.create().buildInstance() if possible. emberjs/ember-test-helpers#237

Merged

Turbo87 mentioned this pull request Nov 8, 2017

Rename to @ember/test-helpers emberjs/ember-test-helpers#240

Closed

Turbo87 reviewed Nov 8, 2017

View reviewed changes

Tweak summary prose for clarity and purpose.

b05ed2c

rwjblue merged commit 1b3e82a into master Dec 1, 2017

rwjblue deleted the acceptance-testing branch December 1, 2017 20:11

This was referenced Dec 16, 2017

Add application testing infrastructure. emberjs/ember-test-helpers#279

Merged

Implement startApplicationTest from emberjs/rfcs#268. emberjs/ember-qunit#302

Merged

rwjblue added a commit to emberjs/ember-qunit that referenced this pull request Dec 17, 2017

Merge pull request #302 from emberjs/application-tests

39f1ac6

Implement `startApplicationTest` from emberjs/rfcs#268.

This was referenced Dec 17, 2017

Ember 2.18 Release Blog Post emberjs/website#3092

Merged

Update acceptance-test blueprint for RFC#268. emberjs/ember.js#15999

Merged

import { Promise } from 'rsvp'; causes simple tests to fail emberjs/ember-qunit#300

Closed

cibernox mentioned this pull request Dec 19, 2017

Refactor test helpers jamesarosen/ember-i18n#468

Closed

rwjblue mentioned this pull request Jan 3, 2018

Usage of initializer causes race condition in tests. ember-spaniel/ember-spaniel#22

Open

rwjblue mentioned this pull request Jan 11, 2018

Remove unused testing helper files. ember-cli/ember-cli#7546

Merged

rwjblue mentioned this pull request Jan 12, 2018

Remove embertest from ESLint configuration. ember-cli/ember-cli#7553

Merged

rwjblue mentioned this pull request Feb 14, 2018

Add qunit-dom dependency ember-cli/rfcs#116

Merged

axsuul mentioned this pull request Feb 26, 2018

Acceptance tests with ember-tether yapplabs/ember-modal-dialog#48

Open

This was referenced Mar 25, 2018

Bump ember-source from 2.18.2 to 3.0.0 ember-lifeline/ember-lifeline#80

Closed

Bump ember-source from 2.15.0 to 3.0.0 rwjblue/ember-test-friendly-error-handler#6

Closed

Bump ember-source from 2.18.0 to 3.0.0 in /ember skylines-project/skylines#754

Closed

GavinJoyce mentioned this pull request Apr 3, 2018

[WIP] Update ember-cli emberjs/ember.js#16459

Closed

1 task

mehulkar mentioned this pull request Apr 8, 2020

Test Directories #575

Closed

Rethink Acceptance Testing #268

Rethink Acceptance Testing #268

Conversation

rwjblue commented Nov 6, 2017

simonihmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cibernox Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtablada commented Nov 8, 2017

Choose a reason for hiding this comment

rwjblue Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

rwjblue commented Nov 8, 2017

rtablada commented Nov 8, 2017

rwjblue commented Nov 8, 2017 • edited Loading

rtablada commented Nov 8, 2017

raido commented Nov 16, 2017

rwjblue commented Dec 1, 2017

simonihmig Nov 6, 2017 •

edited

Loading

rwjblue Nov 6, 2017 •

edited

Loading

cibernox Nov 6, 2017 •

edited

Loading

rwjblue Nov 6, 2017 •

edited

Loading

simonihmig Nov 6, 2017 •

edited

Loading

rwjblue Nov 8, 2017 •

edited

Loading

rwjblue commented Nov 8, 2017 •

edited

Loading