Conversation
active/0000-qunit-dom.md
Outdated
|
||
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. |
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.
Shouldn't the test blueprints (part of ember-source
) also be updated? E.g. here. This would also facilitate the discovery and learning process for users.
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.
Good point, I forgot about that... 🤔
I've updated the text to address this.
active/0000-qunit-dom.md
Outdated
> | ||
> 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 |
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.
We could of course move it to the ember-cli org but as qunit-dom is usable without Ember/Ember CLI I'm not sure that is actually the right place for it.
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.
Ideally it would be an official QUnit plugin on their org where all involved parties have access, but in reality I don't think this will be an issue.
The main point here is that the bus factor should not be 1
for committing and publishing, which is already the case.
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.
IMHO, this isn't really a drawback and in fact it may be a benefit. The most important factors are:
- overall bus factor
- ability of ember-cli team to be able to ship bugfixes / patch releases quickly
Both of those points are addressed by qunit-dom being under simplabs.
active/0000-qunit-dom.md
Outdated
|
||
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. |
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.
Would it make sense to update the blueprints used for generating tests as well as e.g. a component integration test generated with ember g …
currently generates this:
assert.equal(this.element.textContent.trim(), 'template block text');
which (I think) should look like this with qunit-dom
assert.dom(this.element).hasText('template block text');
Without actually updating the blueprints, I'm not sure anyone would even notice qunit-dom is even part of their project and enables nicer tests?
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, I've updated the text to reflect this.
I'm all in favor of this, but I would not add it before qunit-dom has support for mainmatter/qunit-dom#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. |
@corrspt we will certainly iron things out before making it the default. It would be added to the Ember CLI |
active/0000-qunit-dom.md
Outdated
|
||
> Why are we doing this? | ||
|
||
QUnit assertions are quite verbose and can look like this: |
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.
QUnit assertions in and of themselves aren't inherently verbose. I think this should be clarified to something like:
In a modern Ember application making assertions around the state of the DOM is fundamental to confirming that your applications functionality. These assertions are generally quite verbose:
active/0000-qunit-dom.md
Outdated
> | ||
> 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 |
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.
IMHO, this isn't really a drawback and in fact it may be a benefit. The most important factors are:
- overall bus factor
- ability of ember-cli team to be able to ship bugfixes / patch releases quickly
Both of those points are addressed by qunit-dom being under simplabs.
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? |
@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 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 👍 |
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.
🤗 |
We should consider, adding to the alternatives section, adding This route has the advantage of an easier transition and an easier to write codemod. |
@rwjblue Yes, but then the text of this RFC doesn't seem to have heard of it, saying the alternative is this.element.querySelector(...) |
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 |
@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 |
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. |
I am actually against this for a couple of reasons:
I have 2 possible solutions to these concerns:
|
FWIW, I do not think this concern is within the scope of this RFC. The
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 |
It boils down to the question whether people are more likely to look into their 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. |
Definitely not true.
Agree that it is more ugly, but as mentioned above it is not needed. 😄 |
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 😉... Also, note that adding custom methods to |
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 |
@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
I was guessing about start-app.js because in my app, that's where
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 |
do you explicitly inject the Ember Data
please note that adding
IMHO that part belongs in the QUnit documentation, but we can surely link it if it exists there. |
@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 :) |
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! |
> 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 |
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.
we should make sure to document the fact that app devs can opt out by removing the dependency from their package.json
as requested in ember-cli/ember-cli#7605 (comment)
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 :)