From 7745d9e1ee3b999a01f5291581593f38e8481f2b Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 12 Feb 2018 12:53:26 +0100 Subject: [PATCH 1/5] Add `qunit-dom` dependency --- active/0000-qunit-dom.md | 105 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 active/0000-qunit-dom.md diff --git a/active/0000-qunit-dom.md b/active/0000-qunit-dom.md new file mode 100644 index 0000000..a663efb --- /dev/null +++ b/active/0000-qunit-dom.md @@ -0,0 +1,105 @@ +- Start Date: 2018-02-12 +- RFC PR: (leave this empty) + +# Summary + +Introduce [`qunit-dom`] as a dependency by default in the `app` and `addon` blueprints. + +[`qunit-dom`]: https://github.com/simplabs/qunit-dom + +# Motivation + +> Why are we doing this? + +QUnit assertions are quite verbose and can look like this: + +```js +assert.equal(this.element.querySelector('.title').textContent.trim(), 'Hello World!'); +``` + +with `qunit-dom` we can write much more readable assertion for DOM elements: + +```js +assert.dom('.title').hasText('Hello World!'); +``` + +> What use cases does it support? + +It supports the most common assertions on DOM elements, like: + +- what text does the element have? +- what value does the `` element have? +- is a certain CSS class applied to the element + +The full API is documented at . + +> What is the expected outcome? + +Using `qunit-dom` will lead to more simple and readable test code. + + +# Detailed design + +The necessary changes to `ember-cli` are relatively small since we only need +to add the dependency to the `app` blueprint, and the `addon` blueprint will +inherit it automatically. + +This has the advantage (over including it as an implicit dependency), that +apps and addons that don't want to use it for some reason can opt-out by +removing the dependency from their `package.json` file. + +A WIP pull request has been created already at . + + +# How We Teach This + +> Would the acceptance of this proposal mean the Ember guides must be +> re-organized or altered? Does it change how Ember is taught to new users +> at any level? + +Once we decide that this is the right way to go, we should update the official +Ember.js testing guides to use `qunit-dom` assertions by default. This has the +nice side effect of making the testing code in the guides easier to read too. + +> How should this feature be introduced and taught to existing Ember +> users? + +We should also explicitly mention this change in the release blog post and +recommend that people use this from now on. For those users that want to +migrate their existing tests to `qunit-dom` a basic codemod exists at +. + + +# Drawbacks + +> Why should we *not* do this? Please consider the impact on teaching Ember, +> on the integration of this feature with other existing and planned features, +> on the impact of the API churn on existing apps, etc. +> +> There are tradeoffs to choosing any path, please attempt to identify them here. + +- `qunit-dom` is "owned" by a third-party consulting company (simplabs) and + the Ember CLI team is not directly in control. + +- `qunit-dom` has not reached v1.0.0 yet so there might be small breaking + changes in the future. + +- `qunit-dom` is another abstraction layer on top of the raw QUnit assertions + which adds to the existing learning curve. + + +# Alternatives + +> What other designs have been considered? + +None + +> What is the impact of not doing this? + +We will keep using ugly, hard-to-read assertions by default and leave it up +to our users to discover `qunit-dom` by themselves. + + +# Unresolved questions + +None From 40ed9aead72f736dae5ef0b73743bcc00129f5a1 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 12 Feb 2018 13:18:41 +0100 Subject: [PATCH 2/5] qunit-dom: Add paragraphs about `ember-source` blueprint updates --- active/0000-qunit-dom.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/active/0000-qunit-dom.md b/active/0000-qunit-dom.md index a663efb..257468a 100644 --- a/active/0000-qunit-dom.md +++ b/active/0000-qunit-dom.md @@ -61,6 +61,11 @@ Once we decide that this is the right way to go, we should update the official Ember.js testing guides to use `qunit-dom` assertions by default. This has the nice side effect of making the testing code in the guides easier to read too. +At the same time (same minor release) we should update the relevant blueprints +in the `ember-source` package to use `qunit-dom` by default. This should be a +relatively small change as only the `component` and `helper` tests use +DOM assertions. + > How should this feature be introduced and taught to existing Ember > users? @@ -102,4 +107,5 @@ to our users to discover `qunit-dom` by themselves. # Unresolved questions -None +- Should the `ember-source` blueprints detect `qunit-dom` usage and fallback + to raw QUnit assertions if the dependency can't be found? From 5f8e278cf3d177a101c697cbd9f7ef5c598bdd9a Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 15 Feb 2018 12:30:01 +0100 Subject: [PATCH 3/5] qunit-dom: Address feedback --- active/0000-qunit-dom.md | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/active/0000-qunit-dom.md b/active/0000-qunit-dom.md index 257468a..54c9e24 100644 --- a/active/0000-qunit-dom.md +++ b/active/0000-qunit-dom.md @@ -11,13 +11,22 @@ Introduce [`qunit-dom`] as a dependency by default in the `app` and `addon` blue > Why are we doing this? -QUnit assertions are quite verbose and can look like this: +In a modern Ember application making assertions around the state of the DOM is +fundamental to confirming your applications functionality. These assertions are +often quite verbose: ```js assert.equal(this.element.querySelector('.title').textContent.trim(), 'Hello World!'); ``` -with `qunit-dom` we can write much more readable assertion for DOM elements: +Using the `find()` helper of `@ember/test-helpers` we can simplify the DOM +element lookup, but the signal-to-noise ratio of the code is still not great: + +```js +assert.equal(find('.title').textContent.trim(), 'Hello World!'); +``` + +With `qunit-dom` we can write much more readable assertions for DOM elements: ```js assert.dom('.title').hasText('Hello World!'); @@ -97,12 +106,16 @@ migrate their existing tests to `qunit-dom` a basic codemod exists at > What other designs have been considered? -None +- Using the `find()` helper functions can be considered an alternative, but + as mentioned above they still result in more verbose code than using + `qunit-dom`. Another advantage is that `qunit-dom` generates a useful + assertion description by default, while `assert.equal()` will just show + something like "A does not match B". > What is the impact of not doing this? -We will keep using ugly, hard-to-read assertions by default and leave it up -to our users to discover `qunit-dom` by themselves. +We will keep using hard-to-read assertions by default and leave it up to our +users to discover `qunit-dom` by themselves. # Unresolved questions From f97e09ea4d013c3b89a256bfc6da4ef9f62b013e Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Fri, 16 Feb 2018 11:00:00 +0100 Subject: [PATCH 4/5] qunit-dom: Add potential "lock-in" drawback --- active/0000-qunit-dom.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/active/0000-qunit-dom.md b/active/0000-qunit-dom.md index 54c9e24..cfb39a9 100644 --- a/active/0000-qunit-dom.md +++ b/active/0000-qunit-dom.md @@ -100,6 +100,11 @@ migrate their existing tests to `qunit-dom` a basic codemod exists at - `qunit-dom` is another abstraction layer on top of the raw QUnit assertions which adds to the existing learning curve. + +- Adding `qunit-dom` to the default blueprint could make it look even more like + `ember-mocha` is only a second-class citizen. Since we add it to the default + `package.json` file it is easy to opt-out though and can be replaced with + `chai-jquery` or `chai-dom` for a roughly similar API. # Alternatives From c95ac0448fd739fc6caac2d2a9d7c1b89368a492 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 1 Mar 2018 22:26:52 +0100 Subject: [PATCH 5/5] Adjust RFC number --- active/{0000-qunit-dom.md => 0116-qunit-dom.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename active/{0000-qunit-dom.md => 0116-qunit-dom.md} (99%) diff --git a/active/0000-qunit-dom.md b/active/0116-qunit-dom.md similarity index 99% rename from active/0000-qunit-dom.md rename to active/0116-qunit-dom.md index cfb39a9..f8fe2eb 100644 --- a/active/0000-qunit-dom.md +++ b/active/0116-qunit-dom.md @@ -1,5 +1,5 @@ - Start Date: 2018-02-12 -- RFC PR: (leave this empty) +- RFC PR: #116 # Summary