Skip to content

Reset blueprint's ignoredFiles#8240

Merged
kellyselden merged 3 commits intoember-cli:releasefrom
dcyriller:reset-ignoredFiles-blueprint
Dec 6, 2018
Merged

Reset blueprint's ignoredFiles#8240
kellyselden merged 3 commits intoember-cli:releasefrom
dcyriller:reset-ignoredFiles-blueprint

Conversation

@dcyriller
Copy link
Copy Markdown
Contributor

When running ember new or ember addon in sequence, on the same node
process, ignoredFiles gets properly incremented. But it was never reset.

The change this PR introduce is especially usefull in tests.

This PR aims at reseting the key so that the following test setup
works properly:

beforeEach(() => {
  emberNew({ isModuleUnification: true, target: 'addon' })
})

Indeed, a bug occured while testing the addon blueprint for the Module
Unification Quest. Because in the tests, we want to make sure the src
directory exists. Previously, addon directory was disapearing but
the issue remained silent as we were not asserting its existence.

I set the release branch as a target. Because this would unblock ember-cli upgrade in emberjs package (emberjs/ember.js#17212). Not sure if it's appropriate though.

/cc @ppcano

@dcyriller dcyriller changed the title Reset Blueprint model's ignoredFiles Reset blueprint's ignoredFiles Nov 30, 2018
Copy link
Copy Markdown
Member

@kellyselden kellyselden left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'm hesitant to merge something like this without a regression test. Can you find a place to add one?

@dcyriller
Copy link
Copy Markdown
Contributor Author

I've inserted a commit to highlight the bug. The second commit is the fix. I'm going to add a third commit to actually prevent regression with a test.

@dcyriller
Copy link
Copy Markdown
Contributor Author

@kellyselden I added a test. I ended up with: a first commit for the failing test, a second commit for the fix.

@ppcano
Copy link
Copy Markdown
Contributor

ppcano commented Nov 30, 2018

@dcyriller Some tests are failing, try running the tests with the MU flag:

EMBER_CLI_MODULE_UNIFICATION=true node tests/runner.js tests/acceptance/addon-generate-test.js

@dcyriller
Copy link
Copy Markdown
Contributor Author

Thank you @ppcano, it's fixed now

@ppcano
Copy link
Copy Markdown
Contributor

ppcano commented Nov 30, 2018

I have tested this PR, and I confirm this will fix the emberjs/ember.js#17212 test errors

@dcyriller
Copy link
Copy Markdown
Contributor Author

ping @kellyselden

@kellyselden
Copy link
Copy Markdown
Member

Can you rebase one more time? We fixed the remaining AppVeyor issues so it should go green.

When running `ember new` or `ember addon` in sequence, on the same node
process, `ignoredFiles` gets properly incremented. But it's never reset.

This change is especially usefull in tests.

This PR aims at reseting the key so that the following test setup
works properly:
```javascript
beforeEach(() => {
  emberNew({ isModuleUnification: true, target: 'addon' })
})
```

The bug occured while testing the `addon` blueprint for the Module
Unification Quest. Because in the tests, we want to make sure the `src`
directory exists. Previously, `addon` directory was disapearing but
the issue remained silent as we were not asserting its existence.
@dcyriller
Copy link
Copy Markdown
Contributor Author

Great. I rebased, CI is running.

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Dec 4, 2018

Thanks for working on this!

@ppcano
Copy link
Copy Markdown
Contributor

ppcano commented Dec 6, 2018

emberjs/ember.js#17253 finally upgraded Ember to ember-cli@3.5.1 without this fix.

But this fix is needed if we want to remove unnecessary call to fs.ensureDirSync('src') on MU blueprint tests. This PR will fix the changes of emberjs/ember.js#17267

@kellyselden
Copy link
Copy Markdown
Member

Restarted AppVeyor, all green now.

@kellyselden kellyselden merged commit 1eb7950 into ember-cli:release Dec 6, 2018
@dcyriller dcyriller deleted the reset-ignoredFiles-blueprint branch December 6, 2018 15:35
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.

5 participants