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

WIP - Add MU addon tests #8322

Closed
wants to merge 3 commits into from

Conversation

ppcano
Copy link
Contributor

@ppcano ppcano commented Dec 26, 2018

Add tests to validate:

  • MU addon works with Classic dummy app

  • Classic addon works with MU dummy app

The implementation was almost completed, the PR only includes the tests.

In both cases, the MU env variable must be set to run the addon tests.

// It works
EMBER_CLI_MODULE_UNIFICATION=true ember test 
// It does not work
 ember test 

Discussed at #8158

Classic Addon MU Addon
Classic Dummy App EMBER_CLI_MODULE_UNIFICATION
Mu Dummy App EMBER_CLI_MODULE_UNIFICATION EMBER_CLI_MODULE_UNIFICATION

Note

Atm, you cannot run two dummy apps (MU/Classic) in the tests folder. But the new tests validate that the different combinations will work.

If I need two dummy apps in my addon, I would create two dummy folders; for example: dummy and mu-dummy and rename the folders when they want to test different environments (this is not an optimal solution). I have open #8415 to explore a possible solution to this situation.

@ppcano ppcano changed the title Add test: MU addon works with classic dummy app Add MU addon tests Jan 3, 2019
@ppcano ppcano force-pushed the mu-addon-with-classic-dummy branch from ea3f76d to 02b7e99 Compare January 3, 2019 22:52
@ppcano
Copy link
Contributor Author

ppcano commented Jan 3, 2019

I think the tests are failing because of 82bfaaa

@ppcano ppcano closed this Jan 7, 2019
@ppcano ppcano reopened this Jan 7, 2019
@ppcano ppcano force-pushed the mu-addon-with-classic-dummy branch from 02b7e99 to 83b983e Compare January 7, 2019 06:56
@rwjblue
Copy link
Member

rwjblue commented Feb 1, 2019

Looks good to me, would you mind rebasing (to kick off a new CI job) just to make sure things are still green?

@ppcano ppcano force-pushed the mu-addon-with-classic-dummy branch from 83b983e to a45b02a Compare February 1, 2019 21:15
@ppcano
Copy link
Contributor Author

ppcano commented Feb 1, 2019

Rebased, CI job running

@ppcano ppcano closed this Feb 1, 2019
@ppcano ppcano reopened this Feb 1, 2019
@ppcano ppcano closed this Feb 2, 2019
@ppcano ppcano reopened this Feb 2, 2019
@ppcano ppcano closed this Feb 2, 2019
@ppcano ppcano reopened this Feb 2, 2019
@ppcano ppcano force-pushed the mu-addon-with-classic-dummy branch from a45b02a to 11fc124 Compare February 8, 2019 00:01
@ppcano ppcano force-pushed the mu-addon-with-classic-dummy branch from 11fc124 to 16c4d3e Compare February 8, 2019 00:10

// ember-addon
if (this.name === 'dummy') {
testSrcTree = 'src';
destDir = `${this.project.name()}/src`;

// Classic Addon with MU dummy app `kitchen-sink-with-mu-dummy-app` will throw: `Dir not found: src` exception
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any better alternative than dirNotFound to avoid the error? Funnel.allowEmpty does not solve the issue.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that allowEmpty fails to fix the issue here due to the same underlying issue that is described in broccolijs/broccoli-funnel#105.

I'd like to land broccolijs/broccoli-funnel#106 + release, then we can remove this work around.

What do you think?

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 Of course! I follow the issues and when they will be released, I will get rid of the dirNotFound hack.

Copy link
Member

Choose a reason for hiding this comment

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

I just released broccoli-funnel@2.0.2 with the fix...

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 Unfortunately broccoli-funnel@2.0.2 does not fix the issue.

The error is triggered by broccoli/lib/builder before the BroccoliFunnel.build is executed; the BroccoliFunnel.allowEmpty routine is never executed.

Any idea what may happen?

Stack trace:

  - stack: BuilderError: Directory not found: src
    at Builder.checkInputPathsExist (/Users/ppcano/dev/ember/ember-cli/node_modules/broccoli/lib/builder.js:261:15)
    at new Builder (/Users/ppcano/dev/ember/ember-cli/node_modules/broccoli/lib/builder.js:65:10)
    at Builder.setupBroccoliBuilder (/Users/ppcano/dev/ember/ember-cli/lib/models/builder.js:85:24)
    at new Builder (/Users/ppcano/dev/ember/ember-cli/lib/models/builder.js:32:10)
    at ServeTask.run (/Users/ppcano/dev/ember/ember-cli/lib/tasks/serve.js:45:55)
    at Promise.resolve.then (/Users/ppcano/dev/ember/ember-cli/lib/models/command.js:243:46)
    at tryCatcher (/Users/ppcano/dev/ember/ember-cli/node_modules/rsvp/dist/rsvp.js:323:19)
    at invokeCallback (/Users/ppcano/dev/ember/ember-cli/node_modules/rsvp/dist/rsvp.js:495:31)
    at /Users/ppcano/dev/ember/ember-cli/node_modules/rsvp/dist/rsvp.js:559:14
    at flush (/Users/ppcano/dev/ember/ember-cli/node_modules/rsvp/dist/rsvp.js:2402:5)

@ppcano
Copy link
Contributor Author

ppcano commented Feb 8, 2019

@rwjblue CI is green now.

I had to correct the Funnels for the case Classic addon with MU dummy app 16c4d3e (I am asking for possible alternatives)

@ppcano
Copy link
Contributor Author

ppcano commented Feb 15, 2019

Marking as WIP, it may be superceded by #8431

@ppcano ppcano changed the title Add MU addon tests WIP - Add MU addon tests Feb 15, 2019
@ppcano
Copy link
Contributor Author

ppcano commented Feb 19, 2019

Closing in favor of #8431

@ppcano ppcano closed this Feb 19, 2019
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.

None yet

2 participants