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

[BUGFIX beta] Test nested addon import #6043

Merged
merged 1 commit into from Jul 7, 2016

Conversation

xcambar
Copy link
Contributor

@xcambar xcambar commented Jul 6, 2016

refs #5990

This PR updates Addon.prototype.import (https://github.com/ember-cli/ember-cli/pull/6043/files#diff-f1e1923c0440a385f51d2a81c0e53b64R448) as per @machty's implementation (see https://github.com/machty/ember-maybe-import-regenerator/blob/master/index.js#L38-L50).

It also adds a smoke test that builds an app with one addon that itself requires 2 more (hence nested) addons. These addons imports a file each, one with the new this.import(asset), the other with the current app.import(asset).

The PR makes sure that independently from the addon developer's preference (or history), the import binds to the correct app, thanks to @machty's implementation.

@Turbo87 Though I let the smoke test at its initial place, I am more than willing to move it to a more appropriate location, your recommandations are welcome.

As a follow up to #5990, ping @rwjblue and @nathanhammond.

this.timeout(360000);

before(function() {
return createTestTargets(appName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this part here takes quite a long time it might be beneficial to combine this with other "slow" tests. e.g. if there is something like an addons-smoke-test-slow already then you should probably just add it there instead.

@xcambar xcambar force-pushed the test_nested_addon_import branch 4 times, most recently from a559f8d to 6584bc5 Compare July 6, 2016 22:06
@Turbo87 Turbo87 added the bug label Jul 7, 2016
@Turbo87
Copy link
Member

Turbo87 commented Jul 7, 2016

@xcambar I just checked that the current master branch is still passing on Travis. It seems that these test failures in the PR are real and caused by the proposed changes. I haven't looked at what exactly changed yet but it seems to influence some other code paths.

@xcambar
Copy link
Contributor Author

xcambar commented Jul 7, 2016

Thanks for investigating. FWIW, I'm currently trying to split the changes into smaller chunks and see when/why it breaks master.

@rwjblue
Copy link
Member

rwjblue commented Jul 7, 2016

Looks good (and green)! Can you squash down to a single commit, and prefix with [BUGFIX beta] to help ensure we pull this down into the beta branch before releasing 2.7?

@xcambar xcambar changed the title Test nested addon import [BUGFIX] Test nested addon import Jul 7, 2016
@xcambar xcambar changed the title [BUGFIX] Test nested addon import [BUGFIX beta] Test nested addon import Jul 7, 2016
@xcambar
Copy link
Contributor Author

xcambar commented Jul 7, 2016

done

@rwjblue
Copy link
Member

rwjblue commented Jul 7, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Jul 7, 2016

📌 Commit 919c648 has been approved by rwjblue

@homu
Copy link
Contributor

homu commented Jul 7, 2016

⚡ Test exempted - status

@homu homu merged commit 919c648 into ember-cli:master Jul 7, 2016
homu added a commit that referenced this pull request Jul 7, 2016
[BUGFIX beta] Test nested addon import

refs #5990

This PR updates `Addon.prototype.import` (https://github.com/ember-cli/ember-cli/pull/6043/files#diff-f1e1923c0440a385f51d2a81c0e53b64R448) as per @machty's implementation (see https://github.com/machty/ember-maybe-import-regenerator/blob/master/index.js#L38-L50).

It also adds a smoke test that builds an app with one addon that itself requires 2 more (hence nested) addons. These addons imports a file each, one with the new `this.import(asset)`, the other with the current `app.import(asset)`.

The PR makes sure that independently from the addon developer's preference (or history), the import binds to the correct app, thanks to @machty's implementation.

@Turbo87 Though I let the smoke test at its initial place, I am more than willing to move it to a more appropriate location, your recommandations are welcome.

As a follow up to #5990, ping @rwjblue and @nathanhammond.
@xcambar xcambar deleted the test_nested_addon_import branch July 7, 2016 14:58
@Turbo87 Turbo87 added this to the v2.7.0 milestone Jul 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants