Skip to content

Fix missing/incorrect file paths for in-repo addons re-exporting files into the app that are also extended/overridden in the app. #294

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

Conversation

GCheung55
Copy link

@GCheung55 GCheung55 commented Oct 7, 2020

Given a repo with an in-repo-addon that re-exports a module, e.g. service, when the repo has an overriding file of what the in-repo-addon exports, then the overriding file should be included in the lcov file.

E.g., Given the following paths exists in the app

app/services/my-service.js
lib/my-in-repo-addon/addon/services/my-service.js
lib/my-in-repo-addon/app/services/my-service.js

Expected: lcov file should include the following lines.

SF:app/services/my-service.js
SF:lib/my-in-repo-addon/addon/services/my-service.js

Actual: lcov file includes the wrong path, such as the following lines.

SF:lib/my-in-repo-addon/app/services/my-service.js
SF:lib/my-in-repo-addon/addon/services/my-service.js

@GCheung55
Copy link
Author

GCheung55 commented Oct 7, 2020

I thought of two solutions.

  1. Only add the to fileLookup object if the property didn't already exist.
  2. Reverse the order of how the paths are included, which gives app/ paths a higher priority than the addon's app/ paths.

I went with solution 2, as it made more sense to control the order instead of modifying how a property is set and follows how addon app/ and app/ paths are merged by ember-cli.

@GCheung55
Copy link
Author

test-packages/in-repo-addon-test.js is failing because the snapshot is different. I'm not sure how to update the snapshot and if I should update the snapshot.

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.

Thanks for working on this! Only had one small comment, but overall this looks good to me!

…her priority than addon.

rwjblue: "The ordering between in-repo addons and addons is a bit tricky here. I don't think we can guarantee that in-repo addons are "clobbered" by "normal" addons (in fact I think I'd expect the opposite)..."
@rwjblue
Copy link
Collaborator

rwjblue commented Oct 8, 2020

› 1 snapshot failed from 1 test suite. Inspect your code changes or run yarn test -u to update them.

@GCheung55 - Can you do yarn test -u to update the snapshot with the new coverage data?

@GCheung55 GCheung55 requested a review from rwjblue October 8, 2020 20:40
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.

Thanks!

@rwjblue rwjblue merged commit e2adafe into ember-cli-code-coverage:master Oct 8, 2020
@rwjblue rwjblue added the bug label Oct 14, 2020
@rwjblue rwjblue changed the title Fix missing/incorrect file paths in lcov Fix missing/incorrect file paths for in-repo addons re-exporting files into the app that are also extended/overridden in the app. Oct 14, 2020
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.

None yet

2 participants