Deprecate usage of restricted resolver.

[rwjblue]

rendered

This is the first of the RFC's mentioned in the core team notes from 2017-05-26.

[lukemelia]

+1 this was a good idea in principle and a pain in the neck in practice

[bendemboski]

Yes please! Very tired of adding 5 lines of code, including one new injection, to a core service and needing to update dozens of unit test files 😺

[Turbo87]

@rwjblue the mocha examples look correct.

I'm a little worried about that moduleFor() and moduleForComponent() will behave differently now, even though they are both called integration test 🤔

[rwjblue]

@Turbo87 - Can you expand on that a bit? I'm not sure of the difference you are referring to.

After this proposal is implemented the moduleForComponent helper is used for only for tests that will be rendering a template snippet (which has been the most common use for quite some time)...

[rwjblue]

I can update the RFC a bit to make this clearer, but once this deprecation has landed and been absorbed by the community we will be able to remove the requirement to specify integration: true for every test (with a major version bump of ember-qunit / ember-mocha) since it would be the only non-deprecated "type option".

[Turbo87]

@Turbo87 - Can you expand on that a bit? I'm not sure of the difference you are referring to.

right now we have three options for testing components:

• moduleFor() with unit: true
• moduleForComponent() with unit: true (use with --unit)
• moduleForComponent() with integration: true (default)

moduleForComponent() with unit: true can be replaced by moduleFor() with unit: true when you use the component: prefix, and other than that it seems to behave similarly.

moduleFor() with integration: true isn't used anywhere AFAIK.

if we now change everything that was unit: true previously to use integration: true we will end up in a situation where two different types of tests (for components) are both called "integration test", even though they look and behave a bit different.

[rwjblue]

Ah, I see! Thank you for clarifying. I think that is important for me to address in the drawbacks section, but ultimately I believe that the folks would generally not assume two different APIs (moduleFor and moduleForComponent) would have identical behaviors.

This deprecation RFC is not proposing adding new behaviors to any of the main testing functions, and is attempting to reduce the total number of valid options (with the long term goal of removing even integration: true), and therefore reducing the possible options/decision points.

[rwjblue]

Updated drawbacks section with that concern (and the mitigation), as well as added a section RE: removing the integration option at some future point.

[trentmwillis]

Overall, big 👍 on this! I've updated needs more times than I could ever care for.

[trentmwillis]

Do we also deprecate integration during this change? Seems like we should since it will no longer have much significance.

[rwjblue]

Yes, we can deprecate it but it provides somewhat little benefit to deprecate other than forcing all blueprints to change and all tests to be updated (yet again).

I think I'd prefer to avoid the second deprecation...

[trentmwillis]

I definitely understand wanting to avoid the churn, but it feels incorrect to me to leave integration: true sitting in a bunch of tests with no apparent reason for it being there. One concern about this is that even though the flag will no longer do anything, developers may still believe it has some effect on the behavior of the tests.

Maybe rather than introduce a deprecation that logs on every single test that has the flag, we log a message the first time it is encountered? Just to ensure context is given.

[rwjblue]

@trentmwillis - OK, that seems like a reasonable compromise. Basically a single deprecation mentioning that integration: true has no effect and should be removed. I'll update this section to mention that...

Also, for context, I am working on another RFC that will suggest migrating to a new ember-qunit API (see emberjs/ember-qunit#258 for context). My plan is to land both of these RFCs in roughly the same timeframe, so that we can update the blueprints once. The new API will not allow unit, needs, or integration options, and will likely suggest deprecating the current style completely (which is why I am not terribly concerned with leaving integration around for long...).

[rwjblue]

Updated the verbiage here...

[Turbo87]

@rwjblue since this will be a larger refactoring for everyone I'm wondering if this change should be combined with the proposed new testing API... 🤔

[rwjblue]

@Turbo87 - There will be another RFC (hoping to submit today/tomorrow) for that. My plan is to move forward with both things together to avoid needless churn in the blueprints (and mental models).

However, I wanted to discuss both changes independently. One of the changes is a true deprecation (as you can see I used the deprecation template here) and the other is a "new feature".

[rwjblue]

This was discussed at the 2017-06-30 core team meeting, and given that the responses by folks are overwhelmingly positive it is ready to advance to final comment period.

[rwjblue]

No new information was identified during final comment period, thanks to everyone for the detailed review and help working through this. Let's do it!