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

Setup addon tests #171

Merged
merged 1 commit into from
Dec 4, 2018
Merged

Setup addon tests #171

merged 1 commit into from
Dec 4, 2018

Conversation

xg-wang
Copy link
Member

@xg-wang xg-wang commented Nov 21, 2018

Introduce ember-cli-addon-tests to test:

  1. ember app with ember-cli-fastboot has dist/ember-fetch/fastboot-fetch.js built
  2. serve ember app with ember-cli-fastboot renders fetched content

@xg-wang
Copy link
Member Author

xg-wang commented Nov 22, 2018

Blocked by tomdale/ember-cli-addon-tests#198 and tomdale/ember-cli-addon-tests#199

Workarounds:

  1. install a fixed version ember-data rather than master
  2. npm run ember-cli-addon-tests

@xg-wang
Copy link
Member Author

xg-wang commented Nov 28, 2018

@Turbo87 @stefanpenner @rwjblue This is green now, can you review?

pkg.devDependencies['ember-cli-fastboot'] = '*';
// These 2 are in ember-fetch's package.json, symlinking to dummy won't help resolve
pkg.devDependencies['abortcontroller-polyfill'] = '*';
pkg.devDependencies['node-fetch'] = '*';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand. Why are these needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

See this PR, Fastboot.requrie is reading from node_modules/node-fetch but it's under node_modules/ember-fetch/node-fetch when symlink.


let app;

before(function() {
Copy link
Member

Choose a reason for hiding this comment

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

this should use beforeEach() so that the tests can't leak state

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, changed


let app;

before(function() {
Copy link
Member

Choose a reason for hiding this comment

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

this should use beforeEach() so that the tests can't leak state


loadInitializers(App, config.modulePrefix);

export default App;
Copy link
Member

Choose a reason for hiding this comment

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

where are these fixture files being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are used by ember-cli-addon-tests to copy over generated new app. It's same setup as https://github.com/ember-fastboot/ember-cli-fastboot/tree/master/test

@xg-wang xg-wang force-pushed the fastboot branch 3 times, most recently from fcf0c9a to cef993a Compare December 3, 2018 05:55
use npm run instead of yarn in travis to work around
tomdale/ember-cli-addon-tests#198
@@ -32,7 +32,7 @@ jobs:
script:
- yarn lint:js
- yarn test
- yarn test:node
- npm run test:node
Copy link

Choose a reason for hiding this comment

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

Why change this to npm?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a known issue tomdale/ember-cli-addon-tests#198, should be fixed by a WIP PR tomdale/ember-cli-addon-tests#105

@xg-wang xg-wang merged commit 1c3a8d5 into ember-cli:master Dec 4, 2018
@xg-wang xg-wang deleted the fastboot branch December 4, 2018 16:17
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.

3 participants