From e50cacdd9da432469a9105205c4d1a128abb234c Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Sun, 11 Jun 2017 20:29:24 -0400 Subject: [PATCH 1/7] Deprecate usage of restricted resolver. --- ...0-deprecate-testing-restricted-resolver.md | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 text/0000-deprecate-testing-restricted-resolver.md diff --git a/text/0000-deprecate-testing-restricted-resolver.md b/text/0000-deprecate-testing-restricted-resolver.md new file mode 100644 index 0000000000..852fe13e74 --- /dev/null +++ b/text/0000-deprecate-testing-restricted-resolver.md @@ -0,0 +1,188 @@ +- Start Date: 2017-06-11 +- RFC PR: (leave this empty) +- 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? + +# 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 + +The primary drawback to this 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. From 9c0b800c6c393591a298e5c8b4f69c017145e21b Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Sun, 11 Jun 2017 20:38:52 -0400 Subject: [PATCH 2/7] Rename to correct filename. --- ...-resolver.md => 0229-deprecate-testing-restricted-resolver.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{0000-deprecate-testing-restricted-resolver.md => 0229-deprecate-testing-restricted-resolver.md} (100%) diff --git a/text/0000-deprecate-testing-restricted-resolver.md b/text/0229-deprecate-testing-restricted-resolver.md similarity index 100% rename from text/0000-deprecate-testing-restricted-resolver.md rename to text/0229-deprecate-testing-restricted-resolver.md From da3463060f83b03c8150eeb715495f428808e07d Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Sun, 11 Jun 2017 20:39:32 -0400 Subject: [PATCH 3/7] Update RFC PR reference. --- text/0229-deprecate-testing-restricted-resolver.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0229-deprecate-testing-restricted-resolver.md b/text/0229-deprecate-testing-restricted-resolver.md index 852fe13e74..d7ed969ef1 100644 --- a/text/0229-deprecate-testing-restricted-resolver.md +++ b/text/0229-deprecate-testing-restricted-resolver.md @@ -1,5 +1,5 @@ - Start Date: 2017-06-11 -- RFC PR: (leave this empty) +- RFC PR: [emberjs/rfcs#229](https://github.com/emberjs/rfcs/pull/229) - Ember Issue: (leave this empty) # Summary From 9002e70233c168e39e218c85383dc3bbb5655ab6 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Sun, 11 Jun 2017 20:41:09 -0400 Subject: [PATCH 4/7] Fix missing quotes. --- text/0229-deprecate-testing-restricted-resolver.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0229-deprecate-testing-restricted-resolver.md b/text/0229-deprecate-testing-restricted-resolver.md index d7ed969ef1..55de63a496 100644 --- a/text/0229-deprecate-testing-restricted-resolver.md +++ b/text/0229-deprecate-testing-restricted-resolver.md @@ -53,7 +53,7 @@ The migration path for `moduleFor`, `moduleForModel`, `setupTest`, and // ember-qunit // before -moduleFor('service:session); +moduleFor('service:session'); moduleFor('service:session', { unit: true From d37f288160b093a97673ab4787bc86806ee9d30c Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 12 Jun 2017 09:59:25 -0400 Subject: [PATCH 5/7] Add additional drawback/concern. --- ...9-deprecate-testing-restricted-resolver.md | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/text/0229-deprecate-testing-restricted-resolver.md b/text/0229-deprecate-testing-restricted-resolver.md index 55de63a496..dc01a1d29d 100644 --- a/text/0229-deprecate-testing-restricted-resolver.md +++ b/text/0229-deprecate-testing-restricted-resolver.md @@ -179,10 +179,28 @@ valid usages. # Drawbacks -The primary drawback to this 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. +## 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). From fac63128a28ee4b0b34ab13bd4011b806fb9cf10 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 12 Jun 2017 10:38:15 -0400 Subject: [PATCH 6/7] Add section about removing `integration` flag. --- text/0229-deprecate-testing-restricted-resolver.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/text/0229-deprecate-testing-restricted-resolver.md b/text/0229-deprecate-testing-restricted-resolver.md index dc01a1d29d..696ba68557 100644 --- a/text/0229-deprecate-testing-restricted-resolver.md +++ b/text/0229-deprecate-testing-restricted-resolver.md @@ -171,6 +171,17 @@ This includes: * `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 From 2dfd8f3253efb625d70c28c6c0d3b64849500e24 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 13 Jun 2017 14:27:15 -0400 Subject: [PATCH 7/7] Add information about deprecating `integration` option. --- text/0229-deprecate-testing-restricted-resolver.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/text/0229-deprecate-testing-restricted-resolver.md b/text/0229-deprecate-testing-restricted-resolver.md index 696ba68557..888b3dcec1 100644 --- a/text/0229-deprecate-testing-restricted-resolver.md +++ b/text/0229-deprecate-testing-restricted-resolver.md @@ -174,13 +174,16 @@ This includes: ## 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`, +support for the `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. +Once the underlying libraries have done a major version bump, we will +introduce a deprecation for using the `integration` option. This +deprecation would be issued once for the entire test suite (not once +per test module which has `integration` passed in). We will also update +the blueprints to remove the extraneous `integration` option. # How We Teach This