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

Add support for running different dummy apps #8416

Closed
wants to merge 3 commits into from

Conversation

ppcano
Copy link
Contributor

@ppcano ppcano commented Feb 8, 2019

EMBER_CLI_DUMMY=dummy2 ember serve
EMBER_CLI_DUMMY=dummy2 ember test
  • The dummy app must be located at tests/${dummyAppName}
  • The default value of the option is dummy

A non default dummy app must change the default modulePrefix:

//config/environment.js
    modulePrefix: 'dummy2'

A needed additional change is that the DefaultPackager needs to be informed that it is building a Dummy app a7848d6

Related

https://github.com/mansona/ember-cli-multiple-dummy-apps
https://github.com/mansona/testing-mu
#8322
#8158

  • Should the blueprint option (ember g component foo-bar --dummy) be considered?

@NullVoxPopuli
Copy link
Contributor

I think I would much prefer keeping the top-level tests structure as-is, so that the folder doesn't get too polluted with test apps. (polluted is such an incorrect word here, but I don't have anything better). But in the same vein as the Octane structure, I think it makes sense to have a "collection" of dummy apps, as I tried here: #8158

  • <root>/
    • tests/
      • dummy/
        • {app-name1}
          • a classic app
        • {app-name2}
          • a mu app

@ppcano
Copy link
Contributor Author

ppcano commented Feb 8, 2019

I would much prefer keeping the top-level tests structure as-is, so that the folder doesn't get too polluted with test apps.

Yes, this is the disadvantage of this structure. On the other hand, this structure does not require any change to existing apps which may require a new RFC and implementing a migration plan.

I think it makes sense to have a "collection" of dummy apps,

This PR also allows to have a "collection" of dummy apps, but using a different structure.

tests/
├── acceptance
├── dummy
│   ├── app
│   ├── config
│   └── public
├── dummy2
│   ├── app
│   ├── config
│   └── public
├── helpers
├── index.html
├── integration
├── test-helper.js
└── unit

@NullVoxPopuli
Copy link
Contributor

I think it makes sense to have a "collection" of dummy apps,

This PR also allows to have a "collection" of dummy apps, but using a different structure.

by collection, I just meant in the same meaning as the octane layout's "collections" or "private collections" where they are grouped into a folder, such as -components, or routes.

may require a new RFC and implementing a migration plan.

if what I'm doing requires an RFC, then I think this PR would also need an RFC. I feel like we're in a bit of a gray area.

I mean, the plan is always to support the original dummy app.

@stefanpenner
Copy link
Contributor

stefanpenner commented Feb 8, 2019

how about? --dummy=./path/to/alt/dummy This mimics --path for serve and test, and also allows the developer to specify his own folder structure for these alt dummy apps.

@ppcano
Copy link
Contributor Author

ppcano commented Feb 8, 2019

@stefanpenner I started with the same API in mind.

--dummy=./path/to/alt/dummy

Then I realized that the implementation was more complex:

  • more changes in lib/broccoli/ember-addon.js.
  • has to forward commandOptions to emberAddon.options.

This API should not require major changes.

My initial intention was to provide a Proof of Concept and API to start the discussion and validate or discard the proposal.

@kellyselden
Copy link
Member

I've always wanted a way to do this, along with moving the ember-cli-build.js to the dummy (so you can have more than one). Thanks for getting the discussion started!

@gossi
Copy link

gossi commented Feb 9, 2019

I like the approach of @mansona in his testing-mu more than the one proposed here. His idea of giving a location to multiple (other) apps is a big benefit. E.g. you can have a docs app (for either addon, engines or apps - soon™) as well as different apps to test against (classic and octane). Then those apps that are used for testing have a isTestingApp() (similar to isModuleUnification() atm), which explicitely selects them as apps to test against.

process.env.EMBER_ADDON_ENV = process.env.EMBER_ADDON_ENV || 'development';

// `testExclude` only excludes the "running" dummy app (provided by the `name` value),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be reviewed.

@Turbo87
Copy link
Member

Turbo87 commented Feb 27, 2019

I'm a little worried that this might conflict with plans to be able to build multiple dummy apps in parallel. I think @ef4 mentioned something like that? 🤔

We might want to let this go through the RFC process before accepting this as a new feature

@NullVoxPopuli
Copy link
Contributor

I think I'd want to be able to have different sets of acceptance tests per dummy app.

@kellyselden
Copy link
Member

My thoughts are:

tests
├── apps
│   ├── dummy
│   │   ├── app
│   │   ├── config
│   │   ├── public
│   │   └── tests
│   │       └── acceptance
│   └── dummy2
│       ├── app
│       ├── config
│       ├── public
│       └── tests
│           └── acceptance
├── helpers
├── index.html
├── integration
├── test-helper.js
└── unit

@Turbo87
Copy link
Member

Turbo87 commented Feb 27, 2019

@kellyselden that is precisely why I think we need an RFC for this

@kellyselden
Copy link
Member

Strongly agreed.

@ef4
Copy link
Contributor

ef4 commented Feb 27, 2019

See also prior discussion in ember-cli/rfcs#119

@bertdeblock
Copy link
Contributor

Going to close this one for now if that's okay, based on the last comments and the fact that this PR is probably outdated at this time.

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

9 participants