-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Make ember-application
package tests pass without jQuery.
#16023
Conversation
@@ -36,12 +36,13 @@ import { | |||
|
|||
moduleFor('Ember.Application, autobooting multiple apps', class extends ApplicationTestCase { | |||
constructor() { | |||
jQuery('#qunit-fixture').html(` | |||
let fixture = document.querySelector('#qunit-fixture'); |
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.
Update this to use get fixture() {}
022b1ec
to
62c5e6e
Compare
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.
LGTM
@@ -25,6 +23,6 @@ moduleFor('Ember.Application with default resolver and autoboot', class extends | |||
|
|||
['@test templates in script tags are extracted at application creation'](assert) { | |||
this.runTask(() => this.createApplication()); | |||
assert.equal(this.$('#app').text(), 'Hello World!'); | |||
assert.equal(document.querySelector('#app').textContent, 'Hello World!'); |
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.
This can be getElementById
, but we can/should probably just make #app
this.element
so you can just use this.assertText
@@ -655,7 +655,7 @@ moduleFor('The {{link-to}} helper - nested routes and link-to arguments', class | |||
{{#each model as |person|}} | |||
<li> | |||
{{#link-to 'item' person}} |
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.
Why not id=person.id
here too?
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, will update to that.
@@ -43,6 +48,11 @@ export default class AbstractTestCase { | |||
return run.next(callback); | |||
} | |||
|
|||
setupFixture(innerHTML) { | |||
let fixture = document.querySelector('#qunit-fixture'); |
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.
getElementById
?
} | ||
|
||
textValue() { | ||
return this.$().text(); | ||
return this.element ? this.element.textContent : ''; |
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.
does anything break without the guard?
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, some of the visit
tests with shouldRender: false
throw without the guard. Specifically they are trying to confirm that nothing has rendered.
I could remove the guard, and update those handful of tests to simply use assert.equal(document.getElementById('qunit-fixture').children.length, 0)
.
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 see.. maybe put that in an assertNothingRendered
method or something. Perhaps we can just assert this.element = null
.. but I am not sure if that semantically works for all the 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.
It was only one test after the other visit test refactors that I did so I just put an inline assertion there...
8e31709
to
18aafe1
Compare
Leverages work done by ember-native-dom-event-dispatcher addon, and switches implementation based on jQuery presence.
Adds support for running tests per-package without jQuery so that jQuery requirements do not creep into the rest of the framework.
60a0e9e
to
49e6727
Compare
This gets the
ember-application
package test suite passing when jQuery is not loaded.To test a given package without jQuery, navigate to:
Primary changes: