Skip to content

Convert to Monorepo #289

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

Merged
merged 6 commits into from
Sep 21, 2020
Merged

Conversation

Turbo87
Copy link
Collaborator

@Turbo87 Turbo87 commented Sep 17, 2020

😱 😱 😱 😱

no, wait, hear me out!

This PR is inspired by the repository layout that https://github.com/embroider-build/embroider/ is using. Instead of using the barely maintained https://github.com/tomdale/ember-cli-addon-tests project, we instead use a monorepo layout with several fixture packages in a test-packages folder.

This enables us to use these test fixtures for debugging in a much easier way than before because they can also be used outside of running the test suite. In other words you can cd into the folder, run COVERAGE=true ember test there and manually verify the results in the coverage folder.

It also removes the need for running npm install during the test suite because the initial yarn install would have already installed all the necessary dependencies. Locally this seems to significantly speed up the test runs, for some reason they still seem to take roughly the same time on CI though.

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 17, 2020

This PR is inspired by the repository layout that embroider-build/embroider is using. Instead of using the barely maintained tomdale/ember-cli-addon-tests project, we instead use a monorepo layout with several fixture packages in a test-packages folder.

FWIW, ember-cli-fastboot did the same general move recently (as did ember-engines).

Copy link
Collaborator

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Some files should be moved back to the project root:

  • CHANGELOG.md
  • CONTRIBUTING.md
  • RELEASE.md

Should add a top level version of these files:

  • README.md (either duplicating or referencing the main packages/ember-cli-code-coverage/README.md)

Comment on lines 86 to 95
"changelog": {
"repo": "kategengler/ember-cli-code-coverage",
"labels": {
"breaking": ":boom: Breaking Change",
"enhancement": ":rocket: Enhancement",
"bug": ":bug: Bug Fix",
"documentation": ":memo: Documentation",
"internal": ":house: Internal"
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be moved to root

Comment on lines 103 to 120
"release-it": {
"plugins": {
"release-it-lerna-changelog": {
"infile": "CHANGELOG.md",
"launchEditor": true
}
},
"git": {
"tagName": "v${version}"
},
"github": {
"release": true,
"tokenRef": "GITHUB_AUTH"
},
"npm": {
"publish": false
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be removed (in favor of the changes in ember-fastboot/ember-cli-fastboot#783).

@@ -0,0 +1,67 @@
'use strict';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it somewhat odd to have this file "loose" in the same directory as other packages.

Comment on lines +23 to +24
await execa('git', ['clean', '-f', 'my-app-with-in-repo-addon'], { cwd: __dirname });
await execa('git', ['restore', 'my-app-with-in-repo-addon'], { cwd: __dirname });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems vaguely odd to do git operations in tests, can you explain what this is cleaning up for?

Comment on lines +21 to +31
beforeEach(async function () {
await rimraf(`${BASE_PATH}/coverage*`);
await execa('git', ['clean', '-f', 'my-app'], { cwd: __dirname });
await execa('git', ['restore', 'my-app'], { cwd: __dirname });
});

afterEach(async function () {
await rimraf(`${BASE_PATH}/coverage*`);
await execa('git', ['clean', '-f', 'my-app'], { cwd: __dirname });
await execa('git', ['restore', 'my-app'], { cwd: __dirname });
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This prelude seems to be repeated a bunch of times, can we make it a util and replace this in each file with:

setupTest(BASE_PATH);

"browser-test": "COVERAGE=true ember test",
"lint:hbs": "ember-template-lint .",
"lint:js": "eslint .",
"test": "npm run-script lint:js && npm run-script node-test"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This tries to run node-test, but that's gone

@kategengler
Copy link
Collaborator

👍 Very much in favor of this layout, thanks for doing this!

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 21, 2020

Updated to address the various review comments above.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Sep 21, 2020

sorry, had it on the todo list, just didn't have time yet 😩

@rwjblue rwjblue merged commit 7979f03 into ember-cli-code-coverage:master Sep 21, 2020
@rwjblue
Copy link
Collaborator

rwjblue commented Sep 21, 2020

sorry, had it on the todo list, just didn't have time yet 😩

No worries! I was just helpin' out 😸

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