Skip to content

Use in-repo addon's "root" property for includes location#290

Merged
rwjblue merged 19 commits intoember-cli-code-coverage:masterfrom
nsf-open:in-repo-addon-paths
Sep 21, 2020
Merged

Use in-repo addon's "root" property for includes location#290
rwjblue merged 19 commits intoember-cli-code-coverage:masterfrom
nsf-open:in-repo-addon-paths

Conversation

@mdeanjones
Copy link
Contributor

The goal is to instrument in-repo addons that don't reside directly within /lib by using the addon's root property, if available.

Given how long the current implementation has been around I think I might be missing some important context that makes this less viable than a quick glance would suggest.

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.

Overall, this definitely seems like the right direction! I'd like to land #289 first (to avoid that larger PR having too much rebase hell), but we should definitely get this in...

Comment on lines 173 to 174
typeof addon.root === 'string'
? addon.root
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, why would addon.root not be present? I would think it always would...

let addonDir =
typeof addon.root === 'string'
? addon.root
: path.join(this.project.root, 'lib', addon.name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, this seems super odd / not good. Assuming that an addon is in lib/some-name is definitely broken.

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 21, 2020

OK, landed #289 so this is ready for a rebase. Note the testing system has changed a bit (not using ember-cli-addon-tests any more, for example), so the test might need to be tweaked a little more but overall I think the newer system is much easier to understand/work with...

@mdeanjones
Copy link
Contributor Author

mdeanjones commented Sep 21, 2020

Done, and might I add a very pleasant evolution to the testing story with #289. The hesitance of touching anything /lib related was an overabundance of caution. It appears as though addon.root has been there since time immemorial but I didn't want to bork any edge-cases just in case.

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

rwjblue commented Sep 21, 2020

Thank you @mdeanjones!

@rwjblue rwjblue added the bug label Sep 21, 2020
const fs = require('fs-extra');
const fixturify = require('fixturify');

class InRepoAddon {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need this file anymore, right?

@apellerano-pw
Copy link

I have a question on how to exclude an in-repo-addon introduced into the coverage report by this PR.

We have a folder structure like

- client
  - our-app
  - design-system

And in our-app's package.json we specify the in-repo-addon with ../design-system in the ember-addon.paths array.

How can I exclude this in-repo-addon from the coverage report? I've tried even **/design-system/** which i thought would be the glob catch-all but that didn't seem to do it.

jonathanpruvost pushed a commit to concordnow/ember-cli-code-coverage that referenced this pull request Dec 15, 2020
…code-coverage#290)

Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
jonathanpruvost pushed a commit to concordnow/ember-cli-code-coverage that referenced this pull request Dec 15, 2020
…code-coverage#290)

Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
jonathanpruvost pushed a commit to concordnow/ember-cli-code-coverage that referenced this pull request Dec 15, 2020
…code-coverage#290)

Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
jonathanpruvost pushed a commit to concordnow/ember-cli-code-coverage that referenced this pull request Dec 15, 2020
…code-coverage#290)

Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
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.

4 participants

Comments