-
-
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
blueprints/instance-initializers-test: Add RFC232 variants #15945
blueprints/instance-initializers-test: Add RFC232 variants #15945
Conversation
abb48b3
to
5822a13
Compare
5822a13
to
8156867
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.
I’d like to restructure things a bit, I left an example in line would you mind giving that snippet a whirl and confirming that it works properly? I believe it should (barring any typos etc)
|
||
// Replace this with your real tests. | ||
test('it works', function(assert) { | ||
initialize(this.application); |
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.
Instance initializers expect to receive an Ember.ApplicationInstance instance, but this is an Ember.Application instance.
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.
To fix that, you would do something like:
module(‘asdf’, function(hooks) {
hooks.beforeEach(function() {
this.Application = Ember.Application.extend();
this.application = this.Application.create({ autoboot: true });
this.application.instanceInitializer({
name: ‘initializer under test’,
initialize
});
});
hooks.afterEach(function() {
runDestroy(this.instance);
runDestroy(this.application);
});
test(‘it works’, async function(assert) {
this.instance = this.application.buildInstance();
await this.instance.boot();
assert.ok(didTheInitializerWork);
});
});
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.
@rwjblue Ah yeah overall my bad. So with this test, await this.instance.boot();
results in Assertion Failed: You cannot use the same root element (BODY) multiple times in an Ember.Application
. Failing on this line -
assert(`You cannot use the same root element (${rootElement.selector || rootElement[0].tagName}) multiple times in an Ember.Application`, !rootElement.is(ROOT_ELEMENT_SELECTOR)); |
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.
tested on a 2.17 application.
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 you mind pushing up your demo app so I can tinker with it to find the right combo?
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.
Also, I made a really bad mistake in my comment above, it should be { autoboot: false }
(and I had true
in the comment)
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.
OK, just tested locally, and I think that it will work once updating to { autoboot: false }
(sorry about that).
|
||
hooks.beforeEach(function() { | ||
run(() => { | ||
this.application = Application.create(); |
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.
Can you swap this to Application.create({ autoboot: false })
here?
hooks.beforeEach(function() { | ||
run(() => { | ||
this.application = Application.create(); | ||
this.application.deferReadiness(); |
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 shouldn’t be needed with my suggested changes
setupTest(hooks); | ||
|
||
hooks.beforeEach(function() { | ||
run(() => { |
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 shouldn’t need to Ember.run
here, why is this required? Is there an error or something without 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.
Yeah the error I get is the You have turned on testing mode...
error...specifically related to the Application.create() expression.
Awesome, thank you! |
Progress on #15933