-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Deprecate usage of restricted resolver. #229
Merged
rwjblue
merged 7 commits into
emberjs:master
from
rwjblue:deprecate-restricted-resolver
Jul 29, 2017
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e50cacd
Deprecate usage of restricted resolver.
rwjblue 9c0b800
Rename to correct filename.
rwjblue da34630
Update RFC PR reference.
rwjblue 9002e70
Fix missing quotes.
rwjblue d37f288
Add additional drawback/concern.
rwjblue fac6312
Add section about removing `integration` flag.
rwjblue 2dfd8f3
Add information about deprecating `integration` option.
rwjblue File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,217 @@ | ||
- Start Date: 2017-06-11 | ||
- RFC PR: [emberjs/rfcs#229](https://github.com/emberjs/rfcs/pull/229) | ||
- Ember Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
In order to largely reduce the brittleness of tests, this RFC proposes to | ||
remove the concept of artificially restricting the resolver used under | ||
testing. | ||
|
||
# Motivation | ||
|
||
Disabling the resolver while running tests leads to extremely brittle tests. | ||
|
||
It is not possible for collaborators to be added to the object (or one | ||
of its dependencies) under test, without modifying the test itself (even if | ||
exactly the same API is exposed). | ||
|
||
The ability to restrict the resolver is **not** actually a feature of Ember's | ||
container/registry/resolver system, and has posed as significant maintenance | ||
challenge throughout the lifetime of ember-test-helpers. | ||
|
||
Removing this system of restriction will make choosing what kind of test to | ||
be used easier, simplify many of the blueprints, and enable much simpler refactoring | ||
of an applications components/controllers/routes/etc to use collaborating utilties | ||
and services. | ||
|
||
# Transition Path | ||
|
||
## Deprecate Functionality | ||
|
||
Issue a deprecation if `integration: true` is not included in the specified | ||
options for the APIs listed below. This specifically includes specifying | ||
`unit: true`, `needs: []`, or specifying none of the "test type options" | ||
(`unit`, `needs`, or `integration` options) to the following `ember-qunit` | ||
and `ember-mocha` API's: | ||
|
||
* `ember-qunit` | ||
* `moduleFor` | ||
* `moduleForComponent` | ||
* `moduleForModel` | ||
* `ember-mocha` | ||
* `setupTest` | ||
* `setupComponentTest` | ||
* `setupModelTest` | ||
|
||
### Non Component Test APIs | ||
|
||
The migration path for `moduleFor`, `moduleForModel`, `setupTest`, and | ||
`setupModelTest` is very simple: | ||
|
||
```js | ||
// ember-qunit | ||
|
||
// before | ||
moduleFor('service:session'); | ||
|
||
moduleFor('service:session', { | ||
unit: true | ||
}); | ||
|
||
moduleFor('service:session', { | ||
needs: ['type:thing'] | ||
}); | ||
|
||
// after | ||
moduleFor('service:session', { | ||
integration: true | ||
}); | ||
``` | ||
|
||
```js | ||
// ember-mocha | ||
|
||
// before | ||
describe('Session Service', function() { | ||
setupTest('service:session'); | ||
|
||
// ...snip... | ||
}); | ||
|
||
describe('Session Service', function() { | ||
setupTest('service:session', { unit: true }); | ||
|
||
// ...snip... | ||
}); | ||
|
||
describe('Session Service', function() { | ||
setupTest('service:session', { needs: [] }); | ||
|
||
// ...snip... | ||
}); | ||
|
||
// after | ||
|
||
describe('Session Service', function() { | ||
setupTest('service:session', { integration: true }); | ||
|
||
// ...snip... | ||
}); | ||
``` | ||
|
||
The main change is adding `integration: true` to options (and removing `unit` or `needs` | ||
if present). | ||
|
||
### Component Test APIs | ||
|
||
Implicitly relying on "unit test mode" has been deprecated for quite some time | ||
([introduced 2015-04-07](https://github.com/emberjs/ember-test-helpers/pull/38)), | ||
so all consumers of `moduleForComponent` and `setupComponentTest` are specifying | ||
one of the "test type options" (`unit`, `needs`, or `integration`). | ||
|
||
This RFC proposes to deprecate completely using `unit` or `needs` options with | ||
`moduleForComponent` and `setupComponentTest`. The vast majority of component tests | ||
should be testing via `moduleForComponent` / `setupComponentTest` with the `integration: true` | ||
option set, but on some rare occaisions it is easier to use the "unit test" style is | ||
desired (e.g. non-rendering test) these tests should be migrated to using `moduleFor` | ||
/ `setupTest` directly. | ||
|
||
```js | ||
// ember-qunit | ||
|
||
// before | ||
moduleForComponent('display-page', { | ||
unit: true | ||
}); | ||
|
||
moduleForComponent('display-page', { | ||
needs: ['type:thing'] | ||
}); | ||
|
||
// after | ||
|
||
moduleFor('component:display-page', { | ||
integration: true | ||
}); | ||
``` | ||
|
||
```js | ||
// ember-mocha | ||
describe('DisplayPageComponent', function() { | ||
setupComponentTest('display-page', { unit: true }); | ||
|
||
// ...snip... | ||
}); | ||
|
||
describe('DisplayPageComponent', function() { | ||
setupComponentTest('display-page', { needs: [] }); | ||
|
||
// ...snip... | ||
}); | ||
|
||
// after | ||
|
||
describe('DisplayPageComponent', function() { | ||
setupTest('component:display-page', { integration: true }); | ||
|
||
// ...snip... | ||
}); | ||
``` | ||
|
||
## Ecosystem Updates | ||
|
||
The blueprints in all official projects (and any provided by popular | ||
addons) will need to be updated to avoid triggering a deprecation. | ||
|
||
This includes: | ||
|
||
* `ember-source` | ||
* `ember-data` | ||
* `ember-cli-legacy-blueprints` | ||
* Others? | ||
|
||
## Remove Deprecated `unit` / `needs` Options | ||
|
||
Once the changes from this RFC are made, we will be able to remove | ||
support for `unit` and `needs` options from `ember-test-helpers`, | ||
`ember-qunit`, and `ember-mocha`. This would be a "semver major" | ||
version bump for all of the related libraries to properly signal that | ||
functionality was removed. | ||
|
||
We can then update the blueprints to remove the extraneous `integration` | ||
option. | ||
|
||
# How We Teach This | ||
|
||
This RFC would require an audit of the main Ember.js guides to ensure | ||
that all usages of the APIs in question continue to be non-deprecated | ||
valid usages. | ||
|
||
# Drawbacks | ||
|
||
## Churn | ||
|
||
One drawback to this deprecation proposal is the churn associated with | ||
modifying the options passed for each test. This can almost certainly | ||
be mitigated by providing a codemod to enable automated updating. | ||
|
||
There are additional changes being entertained that would require changes | ||
for the default testing blueprints, we should ensure that these RFCs do not | ||
conflict or cause undue churn/pain. | ||
|
||
## `integration: true` Confusion | ||
|
||
Prior to this deprecation we had essentially 4 options for testing components: | ||
|
||
* `moduleFor(..., { unit: true })` | ||
* `moduleFor(..., { integration: true })` | ||
* `moduleForComponent(..., { unit: true })` | ||
* `moduleForComponent(..., { integatrion: true })` | ||
|
||
With this RFC the option `integration` no longer provides value (we aren't talking | ||
about "unit" vs "integration" tests), and may be seen as confusing. | ||
|
||
I believe that this concern is mitigated by the ultimate removal of the `integration` | ||
(it is only required in order to allow us a path forward that is compatible with | ||
todays ember-qunit/ember-mocha versions). |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also deprecate
integration
during this change? Seems like we should since it will no longer have much significance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
, orintegration
options, and will likely suggest deprecating the current style completely (which is why I am not terribly concerned with leavingintegration
around for long...).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the verbiage here...