Add `qunit-dom` dependency

[Turbo87]

as requested in https://github.com/ember-cli/ember-cli/pull/7605#issuecomment-363091174

Rendered

To keep the potential discussions here focused, please use GitHub reactions (👍/ 👎) to express whether you agree or not, unless you want to share your reasons :)

[corrspt]

I'm all in favor of this, but I would not add it before qunit-dom has support for https://github.com/simplabs/qunit-dom/issues/51, or something to see checkboxes for example:

Having this code is better than the alternative of not using qunit-dom, but it's kind of weird.

assert.dom('[data-test-id="both-sides"] label').hasClass('checked');
await click('[data-test-id="consumable-insert"] input');
// NOTE: assert.dom currently does not have a way to check for these properties
assert.ok(this.$('[data-test-id="consumable-insert"] input').prop('checked'));

I've only recently started using qunit-dom, and I like it a lot. Granted I'm not much experienced with testing in general, but I think it these little things should be ironed out, before being added to the documentation/guides of emberjs.

[Turbo87]

@corrspt we will certainly iron things out before making it the default. It would be added to the Ember CLI master branch, which means that it will take between 6 to 12 weeks until the change is actually rolled out in a stable release, which should be enough time to gather more feedback from early users and resolve issues or missing functionality like you mentioned.

[rwjblue]

I'm all in favor of this, but I would not add it before qunit-dom has support for simplabs/qunit-dom#51

This is a bit odd to me. Why would we block making a large number of cases better just because a single (fairly narrow case) isn't yet addressed?

[corrspt]

@rwjblue Hi, thanks for the reply

I see what you mean. It might just be my applications having lot's of checkboxes 😓 ?

In any case, what I was thinking is that updating the documentation to provide these new assertions (which I agree are much better, I'm using them), but then when you want to something simple/common (in my vision at least) you have to revert to using the things you want to reduce makes it.... confusing, especially for new users.

On the other hand, I do understand having 80/90% of use cases with better ergonomics and having the "escape hatch" for the remaining cases to be a good place to be. Since @Turbo87 reminded that it would still take some time for this to be available and by then this situation is probably resolved.... my concerns don't apply anymore.

Also, special thanks on all your work with EmberJS (along side the rest of the Ember core team). I'm really grateful for the awesome tools at my disposal 👍

[rwjblue]

On the other hand, I do understand having 80/90% of use cases with better ergonomics and having the "escape hatch" for the remaining cases to be a good place to be.

Exactly! qunit-dom is awesome at what it does, but it can never handle every possible way you may want to assert against DOM. qunit-dom is "just" a really nice abstraction layer, but as with all abstractions your mileage may vary and you will sometimes need to "drop down a level" to a more basic abstraction.

Also, special thanks on all your work with EmberJS (along side the rest of the Ember core team). I'm really grateful for the awesome tools at my disposal 👍

🤗

[Gaurav0]

We should consider, adding to the alternatives section, adding find, findWithAssert, etc. to @ember/test-helpers just like what is now available in ember-native-dom-helpers, and specifically identify why that isn't enough.

This route has the advantage of an easier transition and an easier to write codemod.

[rwjblue]

@Gaurav0 we already added find and findAll to @ember/test-helpers (see docs here) specifically to ease the transition from ember-native-dom-helpers, but that is pretty unrelated to actually authoring assertions on DOM content...

[Gaurav0]

@rwjblue Yes, but then the text of this RFC doesn't seem to have heard of it, saying the alternative is this.element.querySelector(...)

[rwjblue]

Using this.element.querySelector is preferable to find in the test itself. It makes it extremely clear what is going on (that there is no “magic” above and beyond the DOM itself which is not the case with find), and was the officially proposed API in emberjs/rfcs#268.

[Gaurav0]

@rwjblue ok, ok, no need to argue. I just think using find should be mentioned as an alternative, and let's list the reasons it is not as good as qunit.dom. Just to improve the RFC and complete the discussion. I think those of us used to ember-native-dom-helpers might find helpful the reasons the new way is preferable so as to convince us to switch over. ;)

[Turbo87]

@Gaurav0 @rwjblue @corrspt Thanks for the feedback! I've updated the RFC to address your points.

[kellyselden]

I like this RFC. I think it would make sense in Drawbacks to address any qunit "lock-in" concerns. Does this make is harder for a new app to switch to ember-cli-mocha for example? Probably not, but someone might get the impression that it is "less" supported now that we have more qunit ecosystem in the default setup.

[mehulkar]

I am actually against this for a couple of reasons:

• It is not clear where assert.dom is coming from.

  For someone not familiar with ember's ecosystem or with javascript, it will be a huge pain to chase this down. Even if you know that ember (or is ember-cli??) uses qunit under the hood, googling "qunit assert" will only give you the native API, and not addon API.

• It is not clear where assert is coming from.

  I love the other related RFCs that have put a focus on importing things from the appropriate places. I am looking forward to being able to have a debug path when things go wrong. I already don't like that assert is yielded to the test() function callback instead of being imported. Adding API to this will only make it more confusing.

I have 2 possible solutions to these concerns:

• Add import { assertDom } from 'qunit-dom' to the blueprint instead.

  This isn't pretty (and also I will eventually hate this because I do want to use a single assert object), but I prefer the clarity.

• Add application.injectQunitDomHelpers(); to the tests/start-app.js with a block comment explaining that it will make qunit-dom helpers available, and that users should remove the package from package.json also if they wish to remove this injection.

  I know this is a configuration and may be hard to implement, but putting this in place means that the user needs to learn how The Ember Test Framework™ works, but not so much how ALL of javascript works (package.json, qunit, qunit-dom, ember-cli, etc).

[rwjblue]

It is not clear where assert is coming from.

FWIW, I do not think this concern is within the scope of this RFC. The assert argument is a fundamental part of the QUnit test framework. Other test frameworks (e.g. mocha) have their own specific ways to do assertions.

It is not clear where assert.dom is coming from.

I completely agree with this concern, and have mentioned it in passing in slack.

RE: solutions, I think I would be perfectly happy with an import from qunit-dom directly (over a "magical" method being added to the assert object), but adding anything to Ember.Application is pretty much out the window 😺

[Turbo87]

Add import { assertDom } from 'qunit-dom' to the blueprint instead.

import { assertDom } from 'qunit-dom' has bad ergonomics though because you would have to pass in the assert instance and assertDom(assert, '.whatever').hasText('foo') just looks ugly.

Add application.injectQunitDomHelpers(); to the tests/start-app.js

It boils down to the question whether people are more likely to look into their package.json to figure out where functionality it coming from or a somewhat arbitrary tests/start-app.js file (which doesn't even exist anymore, but I assume you mean tests/test-helper.js).

Ember has the "convention over configuration" motto, which to me means that the user should have to deal with as little boilerplate code as possible. That includes having to do any explicit setup for Ember addons unless strictly necessary for the addon to work.

I agree that discoverability could be better, but I also wouldn't want to lose the other benefits and I think documenting this properly in the official Ember guides will solve a lot of these issues.

[rwjblue]

import { assertDom } from 'qunit-dom' has bad ergonomics though because you would have to pass in the assert instance

Definitely not true. qunit-dom can easily just use QUnit.config.current.assert.

and assertDom(assert, '.whatever').hasText('foo') just looks ugly.

Agree that it is more ugly, but as mentioned above it is not needed. 😄

[Turbo87]

Definitely not true. qunit-dom can easily just use QUnit.config.current.assert.

Please note that that is not officially declared as public API according to http://api.qunitjs.com/config/QUnit.config

[rwjblue]

Please note that that is not officially declared as public API according to http://api.qunitjs.com/config/QUnit.config

True, but that’s likely only a PR away :wink:...

Also, note that adding custom methods to QUnit.assert is not documented here either:

http://api.qunitjs.com/config/QUnit.assert

[Turbo87]

Also, note that adding custom methods to QUnit.assert is not documented here

true, but then if you look at http://qunitjs.com/plugins/ you will see that it is basically the universally accepted way of extending QUnit. Examples include https://github.com/bahmutov/qunit-promises, https://github.com/JamesMGreene/qunit-assert-canvas, ...

I'm not sure I've mentioned it in the RFC itself by qunit-dom is actually a regular QUnit plugin, so IMHO it should use their default patterns. It does act as an Ember addon on top of that, but that is just used for better build pipeline integration and the conventional configuration mentioned above.

[mehulkar]

@rwjblue @Turbo87 I appreciate you taking the time to point me to docs, and yes, I'll concede that there is a trail of breadcrumbs that will take me to the right places when I need it. But my point is that adding something from the qunit ecosystem into the ember ecosystem as a default is going to be confusing. I have no problem with Qunit being the future of Ember's testing story, but it concerns me that I'll have to learn both things very well to be able to work smoothly. I pretty much only use assert.equal and assert.ok right now, and those are much easier to understand than whatever other things that might be added by Qunit plugins down the line.

It boils down to the question whether people are more likely to look into their package.json to figure out where functionality it coming from or a somewhat arbitrary

I was guessing about start-app.js because in my app, that's where application.injectTestHelpers() is. I don't care where the injection happens, but IMO it should happen in the ember ecosystem, NOT the qunit ecosystem.

Ember has the "convention over configuration" motto

I knew this would come up 😛. Adding an injection is an explicit way to add something to the ember ecosystem. It has the added benefit being able to turn it off, but I'm not primarily seeing this as a configuration.

Another suggestion would be to first introduce and teach users how to add their own assert methods on assert and then note that arbitrary packages may also do this.

[Turbo87]

Adding an injection is an explicit way to add something to the ember ecosystem. It has the added benefit being able to turn it off, but I'm not primarily seeing this as a configuration.

do you explicitly inject the Ember Data store service into your app?

I pretty much only use assert.equal and assert.ok right now, and those are much easier to understand than whatever other things that might be added by Qunit plugins down the line.

please note that adding qunit-dom by default does not disable those other assertions. you can still use them the same as before. adding it by default means we can use it to simplify the testing code in the guides though, where we would then of course point out where assert.dom() is coming from.

Another suggestion would be to first introduce and teach users how to add their own assert methods on assert and then note that arbitrary packages may also do this.

IMHO that part belongs in the QUnit documentation, but we can surely link it if it exists there.

[mehulkar]

@Turbo87 I can dig deeper into this if you're interested in chatting further, but I think I have made my point in previous posts already. It sounds like this RFC is too far along to change anyone's mind. If no one else shares my concerns, I'll back off :)

[Turbo87]

We have discussed the remaining points in the weekly Ember CLI call and decided that ultimately it will be better to have it than to not have it. Thanks for the feedback and discussion everyone!