Skip to content
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

Merged
merged 2 commits into from
Dec 10, 2017

Conversation

snewcomer
Copy link
Contributor

Progress on #15933

@snewcomer snewcomer force-pushed the instance-initializer-rfc232 branch 2 times, most recently from abb48b3 to 5822a13 Compare December 8, 2017 16:50
Copy link
Member

@rwjblue rwjblue left a 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);
Copy link
Member

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.

Copy link
Member

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);
  });
});

Copy link
Contributor Author

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));

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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)

Copy link
Member

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();
Copy link
Member

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();
Copy link
Member

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(() => {
Copy link
Member

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?

Copy link
Contributor Author

@snewcomer snewcomer Dec 9, 2017

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.

@rwjblue
Copy link
Member

rwjblue commented Dec 10, 2017

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants