[FEATURE] Support for ApplicationInstances in test helpers #12310

trentmwillis · 2015-09-08T22:16:39Z

Addresses #12277. Adds the following:

Application#buildTestInstance, which creates a new instance of an application and registers test helpers on it.
ApplicationInstance#injectTestHelpers and ApplicationInstance#removeTestHelpers which are similar to the same name methods on Application, but bind the test helpers to the instance

Todo:

Discourage use of globals (by not supporting them in instance-based tests)
Tests (unsure how much coverage to include, as we could theoretically duplicate all the tests that use the current setup)

trentmwillis · 2015-09-08T22:18:07Z

packages/ember-testing/lib/test.js

+});
+
+EmberApplicationInstance.reopen({
+  injectTestHelpers() {


This function and ApplicationInstance#removeTestHelpers are super similar to the same functions on Application, I was unsure if I should extract them out though as they differ on a couple lines

trentmwillis · 2015-09-09T21:50:11Z

@stefanpenner @rwjblue this is ready for an initial review.

Primary concern I have is the extent to test the instance-based approach, as I could duplicate all the current test helper tests, but that seems a bit ridiculous.

rwjblue · 2015-09-10T04:31:29Z

packages/ember-testing/lib/helpers.js


+function currentRouteName(app) {
+  var appController = lookup(app, 'controller:application');


Unrelated to this PR directly, but we should update these to use service:-routing instead of controller:application.

I'll open a separate PR for that once this work is finished.

rwjblue · 2015-09-10T04:43:11Z

A few notes:

This should be feature flagged, as it adds new public API.
I would prefer to check something like app.isApplicationInstance instead of using instanceof.
I'm not a giant fan of instance.visit, I would prefer to just go with instance.testHelpers.visit and allow other test helpers to provide the sugar:

// in ember-qunit or ember-test-helpers
for (var helperName in this.instance.testHelpers) {
  this[helperName] = function() {
    this.instance.testHelpers[helperName](...arguments);
  }
}

// allows the following

test('visted foo', function(assert) {
  this.visit('foo');
});

trentmwillis · 2015-09-10T16:18:16Z

I'll make those changes. I think leaving the mapping of the helpers to a more convenient location up to other libraries is fine. Chances are this.instance.visit will be modified to something like this.visit in almost all cases anyway.

trentmwillis · 2015-09-11T00:22:18Z

@rwjblue updated with your requested changes. Also duplicated the current acceptance_test file to do the same tests but in the new format. This uncovered a good number of issues that I've since addressed.

There is one issue that I can't seem to solve, and that is the test with a redirect in it keeps failing. It doesn't seem to pick up on the intermediate transition and thus exits the test before all the assertions have been made. I'll keep digging, but would appreciate any thoughts on the matter.

trentmwillis · 2015-09-11T02:25:11Z

Figured out the last issue, was calling the wrong handleURL method. Everything's green now.

stefanpenner · 2015-09-13T15:50:59Z

I'm not a giant fan of instance.visit, I would prefer to just go with instance.testHelpers.visit and allow other test helpers to provide the sugar:

ah this was my question, so we follow up with ember-test-helpers PR to make this fancy.

stefanpenner · 2015-09-13T15:55:28Z

packages/ember-testing/lib/test.js

+
+      this.testHelpers = {};
+      for (var name in helpers) {
+        this.testHelpers[name] = helper(this, name);


I'm not really a fan of all this copying, but without a large refactoring i dont see a quick fix.

One idea, is rather then copying, the test frameworks gives us a TestHelpers object, something like

this.testHelpers = new TestHelpers(applicationInstance); // internally, this.testHelpers now has a reference to the app instance, and can just do the right thing internally.

stefanpenner · 2015-09-13T15:56:34Z

this LGTM

trentmwillis · 2015-09-15T18:00:56Z

@rwjblue think this can this get merged?

rwjblue · 2015-09-15T19:03:17Z

@trentmwillis - Will try to review in detail tonight, sorry for the delays.

trentmwillis · 2015-09-28T17:35:44Z

Rebased to get the latest changes in. @rwjblue any time to check this out soon?

stefanpenner · 2015-09-28T17:50:47Z

Just did another pass, this looks good. My existing comments stands, but we can always address that later.

It's important somewhere, to bring attention to the fact that we don't (at least that is my understandating) actually intend the end user to use instance.testHelpers.click (atleast in most cases) the eventual goal is for the test helpers to sugar that to this.click etc.

stefanpenner · 2015-09-29T02:07:14Z

we may have some overlap with: #12394

@chancancode r?

trentmwillis · 2015-12-30T19:47:32Z

@stefanpenner @rwjblue had some time to revisit this and update it to take into consideration @chancancode's changes. Hoping we can finally get this in soon.

trentmwillis · 2016-01-12T05:04:46Z

Looks like I finally managed to get the tests passing (other than one version of IE that timed out on Sauce Labs).

I'm not 100% sure why, but the ember-extension-support/tests/data_adapter_test.js file wasn't passing until I setup some of the Application calls in run-loops. Might have something to do with introducing new reopens?

homu · 2016-01-20T03:36:42Z

☔ The latest upstream changes (presumably #12766) made this pull request unmergeable. Please resolve the merge conflicts.

homu · 2016-01-29T08:39:59Z

☔ The latest upstream changes (presumably #12772) made this pull request unmergeable. Please resolve the merge conflicts.

trentmwillis · 2016-02-01T18:50:49Z

@stefanpenner or @rwjblue rebased again, can either of you take a look at this? Or if it's been decided to not support this use case I'll close the PR

nickiaconis · 2016-02-02T23:23:30Z

+1 Would love to see this get merged. Watching Trent's local copy of Ember, with this change applied, cruise through acceptance tests while my default copy slowly churned through those same tests was eye opening.

homu · 2016-02-05T20:11:40Z

☔ The latest upstream changes (presumably #12575) made this pull request unmergeable. Please resolve the merge conflicts.

stefanpenner · 2016-02-05T20:52:47Z

Let me give it a read. I do believe we want this, sorry for letting it slip

stefanpenner · 2016-02-05T20:54:56Z

packages/ember-testing/lib/test.js

+      Example:
+
+      ```javascript
+      var instance = run(App, 'buildTestInstance');


i wonder if we should do a run.join inside of this method, so we don't need to enforce the external run.

I'd be fine with that. Currently chose this approach to keep it similar to how start-app works.

stefanpenner · 2016-02-05T20:57:21Z

@tomdale / @wycats needs your review

rwjblue · 2016-02-05T21:55:12Z

I'd like to have a meeting to discuss with @trentmwillis, @mmun, and I. Basically, I'd rather focus on implementing a new wholistic testing setup (will try to write the RFC tonight), and eventually completely remove/deprecate packages/ember-testing after a migration and wind down period (certainly supported through 3.0.0 though).

trentmwillis · 2016-02-05T22:15:43Z

@rwjblue I think that is a good goal, though I would like to see this feature come sooner rather than later. I do, however, understand wanting to keep churn down and not introduce multiple major testing changes. So, let's talk sometime soon.

tomdale · 2016-02-08T16:31:55Z

@rwjblue @trentmwillis One constraint I'd love for you guys to take into account when designing a holistic new test API is to not surface the concept of "application instance" to new developers. It's just a concept that is very useful for internals but adds complexity to the end user.

As discussed above, just do this.visit() instead of this.instance.visit(). The word "instance" should not appear anywhere; the user already knows they are testing an instance of their application, and how Ember chooses to represent that internally should stay insider baseball.

rwjblue · 2016-02-08T21:28:00Z

The word "instance" should not appear anywhere

Yes, agreed. It is a mega troll that we have Application instances and ApplicationInstance instances. We should not force the mental pain upon anyone else.

trentmwillis · 2016-03-01T00:40:50Z

Closing this in favor of emberjs/rfcs#119.

trentmwillis reviewed Sep 8, 2015
View reviewed changes

trentmwillis force-pushed the instance-test-helpers branch 2 times, most recently from a89a98d to 60582b1 Compare September 9, 2015 21:44

rwjblue reviewed Sep 10, 2015
View reviewed changes

trentmwillis force-pushed the instance-test-helpers branch 2 times, most recently from 6aad2b2 to 62c3adb Compare September 11, 2015 00:19

trentmwillis force-pushed the instance-test-helpers branch from 62c3adb to 1e8568e Compare September 11, 2015 02:16

trentmwillis changed the title ~~[WIP] Add support for ApplicationInstances in test helpers~~ [FEATURE] Support for ApplicationInstances in test helpers Sep 11, 2015

trentmwillis mentioned this pull request Sep 13, 2015

Improve testing performance - use buildInstance to avoid recreating the application ember-cli/ember-cli#4504

Closed

stefanpenner reviewed Sep 13, 2015
View reviewed changes

trentmwillis force-pushed the instance-test-helpers branch 2 times, most recently from c1ff82e to 2042c6b Compare September 28, 2015 17:22

trentmwillis force-pushed the instance-test-helpers branch from 2042c6b to a7941d1 Compare December 30, 2015 19:36

trentmwillis mentioned this pull request Jan 1, 2016

Use routing service instead of application controller in test helpers #12766

Merged

trentmwillis force-pushed the instance-test-helpers branch 5 times, most recently from ab35dab to 4bcef8e Compare January 11, 2016 19:51

trentmwillis force-pushed the instance-test-helpers branch from 4bcef8e to 50f7799 Compare January 12, 2016 04:44

trentmwillis force-pushed the instance-test-helpers branch 2 times, most recently from fd4166e to 693c651 Compare January 20, 2016 04:47

Trent Willis added 2 commits February 1, 2016 10:04

Add support for ApplicationInstances in test helpers

11c1392

Add run-loops to data adapter tests to fix test race condition

c1c881d

trentmwillis force-pushed the instance-test-helpers branch from 693c651 to c1c881d Compare February 1, 2016 18:19

stefanpenner reviewed Feb 5, 2016
View reviewed changes

trentmwillis closed this Mar 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Support for ApplicationInstances in test helpers #12310

[FEATURE] Support for ApplicationInstances in test helpers #12310

trentmwillis commented Sep 8, 2015

trentmwillis Sep 8, 2015

trentmwillis commented Sep 9, 2015

rwjblue Sep 10, 2015

trentmwillis Sep 10, 2015

rwjblue commented Sep 10, 2015

trentmwillis commented Sep 10, 2015

trentmwillis commented Sep 11, 2015

trentmwillis commented Sep 11, 2015

stefanpenner commented Sep 13, 2015

stefanpenner Sep 13, 2015

stefanpenner commented Sep 13, 2015

trentmwillis commented Sep 15, 2015

rwjblue commented Sep 15, 2015

trentmwillis commented Sep 28, 2015

stefanpenner commented Sep 28, 2015

stefanpenner commented Sep 29, 2015

trentmwillis commented Dec 30, 2015

trentmwillis commented Jan 12, 2016

homu commented Jan 20, 2016

homu commented Jan 29, 2016

trentmwillis commented Feb 1, 2016

nickiaconis commented Feb 2, 2016

homu commented Feb 5, 2016

stefanpenner commented Feb 5, 2016

stefanpenner Feb 5, 2016

trentmwillis Feb 5, 2016

stefanpenner commented Feb 5, 2016

rwjblue commented Feb 5, 2016

trentmwillis commented Feb 5, 2016

tomdale commented Feb 8, 2016

rwjblue commented Feb 8, 2016

trentmwillis commented Mar 1, 2016


		function currentRouteName(app) {
		var appController = lookup(app, 'controller:application');

[FEATURE] Support for ApplicationInstances in test helpers #12310

[FEATURE] Support for ApplicationInstances in test helpers #12310

Conversation

trentmwillis commented Sep 8, 2015

trentmwillis Sep 8, 2015

Choose a reason for hiding this comment

trentmwillis commented Sep 9, 2015

rwjblue Sep 10, 2015

Choose a reason for hiding this comment

trentmwillis Sep 10, 2015

Choose a reason for hiding this comment

rwjblue commented Sep 10, 2015

trentmwillis commented Sep 10, 2015

trentmwillis commented Sep 11, 2015

trentmwillis commented Sep 11, 2015

stefanpenner commented Sep 13, 2015

stefanpenner Sep 13, 2015

Choose a reason for hiding this comment

stefanpenner commented Sep 13, 2015

trentmwillis commented Sep 15, 2015

rwjblue commented Sep 15, 2015

trentmwillis commented Sep 28, 2015

stefanpenner commented Sep 28, 2015

stefanpenner commented Sep 29, 2015

trentmwillis commented Dec 30, 2015

trentmwillis commented Jan 12, 2016

homu commented Jan 20, 2016

homu commented Jan 29, 2016

trentmwillis commented Feb 1, 2016

nickiaconis commented Feb 2, 2016

homu commented Feb 5, 2016

stefanpenner commented Feb 5, 2016

stefanpenner Feb 5, 2016

Choose a reason for hiding this comment

trentmwillis Feb 5, 2016

Choose a reason for hiding this comment

stefanpenner commented Feb 5, 2016

rwjblue commented Feb 5, 2016

trentmwillis commented Feb 5, 2016

tomdale commented Feb 8, 2016

rwjblue commented Feb 8, 2016

trentmwillis commented Mar 1, 2016