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

[QUEST] Decouple Ember tests from the globals resolver #15058

Closed
mixonic opened this Issue Mar 23, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@mixonic
Member

mixonic commented Mar 23, 2017

(lifted some of this introduction from #15055)

The Ember test suite is very coupled to the default globals-mode resolver. For example:

  • Tests often make a controller available to an application as App.FooController
  • Tests often use setTemplates to set a template onto Ember.TEMPLATES
  • Even more tests use App.Router to set location or other configuration

All of these ways of passing things to Ember are considered part of the globals-mode resolver API.

At some far future date, we would like to drop the default resolver and its globals API from Ember. Nearly all (but not all) Ember apps are built using Ember-CLI. Those applications (with a few minor tweaks) do not need the default resolver. Already they use the ember-resolver, however this itself is a subclass of the default resolver and retains globals loading as a fallback behavior.

The first step (of many) toward removal of the globals-mode default resolver is to refactor Ember's test suite to not be reliant upon it. To this end #15055 introduced a configurable resolver mock to be used in Ember tests, ideally through the class-based testing abstraction added during the Glimmer2 test porting effort. For example:

/*
 * internal-test-helpers provides several testcase classes.
 */
import { moduleFor, ApplicationTestCase } from 'internal-test-helpers';

/*
 * Pass an anonymous class extending from the testcase to moduleFor.
 */
moduleFor('Link-to component', class extends ApplicationTestCase {

  /* 
   * As you are defining an ES class for your test module, you may
   * override APIs from the parent class and call super. If needed, you
   * can even create a whole new test case in internal-test-helpers
   */

  /*
   * In this format, tests are any property on the class that starts
   * with "@test".
   */
  ['@test should be able to be inserted in DOM when the router is not present']() {
    /*
     * Inside the test the class instance is `this`. For example
     * 
     *     this.addTemplate('application', '<p>{{prop}}</p>');
     *
     * Will add a template to the test resolver. To add a controller
     * you would use the specifier and `add`:
     *
     *     this.add('controller:application', Controller.extend());
     *
     * This replaces older test styles where App.ApplicationController
     * would be set.
     *
     * Exactly what the API of `this` is changes based on what class
     * is extended, of course.
     *
     * Here is a real link-to test as an example: 
     */
    this.addTemplate('application', `{{#link-to 'index'}}Go to Index{{/link-to}}`);

    return this.visit('/').then(() => {
      this.assertText('Go to Index');
    });
  }

});

Roughly taken from link-to-test.js.

ApplicationTestCase will default to the ModuleBasedTestResolver mock. To break it down:

Our goal is to migrate all tests in Ember that implicitly rely on the globals resolver to this improved testing style. Note that the aim really is to use the test resolver: Some tests may be avoiding the implicit default resolver by registering factories into the registry. Those tests should also be migrated to the mock resolver.

How to help

Figuring out what tests use the default resolver to pass is a little tricky since it is, well, default. Once you identify a test that uses the default resolver, you still must ascertain if that test is testing the default resolver or if it the dependency is only implicit. Most tests using the default resolver should be candidates for refactoring: They are not testing interaction with the resolver.

Running this ag call:

ag -H -C 1 -G test.js "setTemplate|\.Router|App\." packages/

Spits out a list of tests possibly using the globals resolver. I've narrowed that list to 28 files that look like good candidates for refactoring:

Your mission, should you choose to accept it, is to pick a file from that list and port it from the old style to a test that uses the class-based testing framework and does not use the global resolver.

A successfully converted file will:

  • Use the class-based testing harness. It may not use ApplicationTestCase though. In fact, there are already other test cases that might be more appropriate such as RenderTestCase or AutobootApplicationTestCase. However, all these class-based systems should default to the TestResolver or ModuleBasedTestResolver. If you add or modify a test case, be sure it defaults to the test resolver.
  • Not be reliant upon the globals mode resolver, unless part of the file is actively testing the default global Resolver. This subset of tests should opt-in to the global resolver. For an example of this see application_test.js:193.
  • Not skirt the matter by using the registry for factories. Unless a test is actively testing interaction with the registry, the test resolver is preferred.

The following are useful resources in your journey:

  • test-resolver.js from internal-test-helpers.
  • test-cases/ folder in internal-test-helpers.
  • #15055 for context and examples of properly migrated files.
  • #15060 as simple example of a conversion by @josemarluedke, plus a note on how autoboot is used in these tests.
  • #13127, the Glimmer 2 test issue, for more examples of the class-based tests. You can poke around the codebase as well.

Please see the Google spreadsheet "Ember tests using the globals resolver" to learn what files make good candidates for this work. Add your Github handle and the date there if you are attempting a refactor. Advanced Jedi may find refactoring opportunities not included in that spreadsheet, please feel free to add them and open a PR.

Thank you for your help! If you're attending EmberConf, perhaps I can pick up a ☕️ or 🍻 tab!

@pythonquick

This comment has been minimized.

pythonquick commented Apr 3, 2017

i'm having some challenges converting the test case to_string_test.js.
I listed one question below, but first some background.

Converting the following line seems easy enough:

App.Post = EmberObject.extend();

with:

this.add('model:post', EmberObject.extend());

in the new ApplicationTestCase sub-class constructor.

The challenging part comes with resolving and looking up this model. The test-resolver.js has a resolve method that can be used to retrieves this model class:

  let PostFactory = this.resolver.resolve('model:post');

The PostFactory variable will now contain the class but calling its toString method results in the string '(unknown mixin)' and the original test case expects 'App.Post'.

The original to_string_test.js file uses the following two App.__container __ method calls:

  PostFactory = App.__container__.factoryFor('model:post').class;
  // ... lines omited
  let post = App.__container__.lookup('model:post');

It looks like the test-resolver's resolve method alone does not suffice for this test case.

So, finally my question:
What are the new-style (non-global) methods to use in place of these factoryFor and lookup methods?

Any tips would be greatly appreciated!

@mixonic

This comment has been minimized.

Member

mixonic commented Apr 3, 2017

@pythonquick you've ended up in a non-trivial part of the migration. The to_string_test.js file is testing the interaction of the registry and resolver makeToString API with factories. In container.js NAME_KEY is set as the value from this resolver or registry hook, and in core_object.js that value is used in toString. I would like to see the to_string_test.js file moved to the new class-based test format, however it can continue to use the globals resolver as the tests are actually testing that makeToString implementation.

Also, for you general reference, factoryFor and lookup are methods on the container. As the resolver is something used by the container internally, the factoryFor and lookup methods will continue to be used and be appropriate regardless of what resolver is used.

@pythonquick

This comment has been minimized.

pythonquick commented Apr 7, 2017

@mixonic thank you for the notes! I'll try to revisit to_string_test.js later on.

Currently I'm trying to convert component_registration_test.js which looks easy enough.
The tricky part comes when dealing with deprecation warnings which raise an exception.

The old-style test deals with this by wrapping the expected deprecation in a call to expectDeprectation.

Unfortunately there is no counterpart assert.expectDeprecation method, except when installing the ember-qunit-assert-helpers addon.

Is there another way of dealing with expected deprecations in the new-style test case?

mixonic added a commit to mixonic/ember.js that referenced this issue Jun 8, 2017

Decouple acceptance_test, helpers_test frm globals
1) Decouple the acceptance_test and helpers_test files from the globals
resolver per emberjs#15058.
2) Update setupForTesting to reference the router factory in the registry
instead of the globally resolved one on the application instance.
3) Extend the Router during application registry setup to isolate
`reopen` calls.

mixonic added a commit to mixonic/ember.js that referenced this issue Jun 8, 2017

mixonic added a commit to mixonic/ember.js that referenced this issue Jun 8, 2017

mixonic added a commit to mixonic/ember.js that referenced this issue Jun 8, 2017

mixonic added a commit to mixonic/ember.js that referenced this issue Jun 10, 2017

mixonic added a commit that referenced this issue Jun 10, 2017

patricksrobertson added a commit to patricksrobertson/ember.js that referenced this issue Jul 3, 2017

Decouple substates tests from the global resolver.
I've moved this into three different test modules:

* A default state that relies on the visit API.
* A nested route module that has shared setup for roughly half the tests
* An AutobootTestCase module for dealing with the one test case that
cannot be worked around.

I've removed the step testing function. This seemed to be more important
for debugging the authoring of the original sets of tests instead of
actually asserting code.

The instance of 'BRO' has been replaced with 'TURTLE' because I hate fun
and value inclusivity.

Refs emberjs#15058

patricksrobertson added a commit to patricksrobertson/ember.js that referenced this issue Jul 3, 2017

Decouple substates tests from the global resolver.
I've moved this into three different test modules:

* A default state that relies on the visit API.
* A nested route module that has shared setup for roughly half the tests
* An AutobootTestCase module for dealing with the one test case that
cannot be worked around.

I've removed the step testing function. This seemed to be more important
for debugging the authoring of the original sets of tests instead of
actually asserting code.

The instance of 'BRO' has been replaced with 'TURTLE' because I hate fun
and value inclusivity.

Refs emberjs#15058

patricksrobertson added a commit to patricksrobertson/ember.js that referenced this issue Jul 5, 2017

Decouple substates tests from the global resolver.
I've moved this into three different test modules:

* A default state that relies on the visit API.
* A nested route module that has shared setup for roughly half the tests
* An AutobootTestCase module for dealing with the one test case that
cannot be worked around.

The instance of 'BRO' has been replaced with 'TURTLE' because I hate fun
and value inclusivity. Also in the service of inclusivity 'smells' has
been replaced by 'puppies'

Refs emberjs#15058

patricksrobertson added a commit to patricksrobertson/ember.js that referenced this issue Jul 6, 2017

Decouple substates tests from the global resolver.
I've moved this into three different test modules:

* A default state that relies on the visit API.
* A nested route module that has shared setup for roughly half the tests
* An AutobootTestCase module for dealing with the one test case that
cannot be worked around.

The instance of 'BRO' has been replaced with 'TURTLE' because I hate fun
and value inclusivity. Also in the service of inclusivity 'smells' has
been replaced by 'puppies'

Refs emberjs#15058

patricksrobertson added a commit to patricksrobertson/ember.js that referenced this issue Jul 14, 2017

Decouple substates tests from the global resolver.
I've moved this into three different test modules:

* A default state that relies on the visit API.
* A nested route module that has shared setup for roughly half the tests
* An AutobootTestCase module for dealing with the one test case that
cannot be worked around.

The instance of 'BRO' has been replaced with 'TURTLE' because I hate fun
and value inclusivity. Also in the service of inclusivity 'smells' has
been replaced by 'puppies'

Refs emberjs#15058

patricksrobertson added a commit to patricksrobertson/ember.js that referenced this issue Jul 14, 2017

Decouple substates tests from the global resolver.
I've moved this into three different test modules:

* A default state that relies on the visit API.
* A nested route module that has shared setup for roughly half the tests
* An AutobootTestCase module for dealing with the one test case that
cannot be worked around.

The instance of 'BRO' has been replaced with 'TURTLE' because I hate fun
and value inclusivity. Also in the service of inclusivity 'smells' has
been replaced by 'puppies'

Refs emberjs#15058

mixonic added a commit that referenced this issue Aug 16, 2017

Decouple substates tests from the global resolver.
I've moved this into three different test modules:

* A default state that relies on the visit API.
* A nested route module that has shared setup for roughly half the tests
* An AutobootTestCase module for dealing with the one test case that
cannot be worked around.

The instance of 'BRO' has been replaced with 'TURTLE' because I hate fun
and value inclusivity. Also in the service of inclusivity 'smells' has
been replaced by 'puppies'

Refs #15058

@acorncom acorncom added the Quest label Jan 10, 2018

@acorncom acorncom changed the title from [EPIC] Decouple Ember tests from the globals resolver to [QUEST] Decouple Ember tests from the globals resolver Jan 10, 2018

@mixonic

This comment has been minimized.

Member

mixonic commented Mar 5, 2018

This is complete! 🎉

And we did it in under a year. 💪

Thank you @patricksrobertson @fsmanuel @josemarluedke @NullVoxPopuli @karthiick @rwjblue @Gaurav0 @stevenwu @lbaillie @ballPointPenguin @defraz @cibernox @pythonquick (I apologize if I missed anyone) for your individual efforts on this huge undertaking. This was a big task and none of us could have completed it alone. You've exemplified what good open source contribution and collaboration looks like.

🙇

@mixonic mixonic closed this Mar 5, 2018

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