Skip to content

Fix no coverage reported for files in app directory of in-repo addons#342

Closed
robinborst95 wants to merge 2 commits intoember-cli-code-coverage:masterfrom
robinborst95:fix/in-repo-addon-app
Closed

Fix no coverage reported for files in app directory of in-repo addons#342
robinborst95 wants to merge 2 commits intoember-cli-code-coverage:masterfrom
robinborst95:fix/in-repo-addon-app

Conversation

@robinborst95
Copy link
Contributor

@robinborst95 robinborst95 commented Nov 16, 2021

In this PR I first added a failing test to confirm this issue. Then I tried diving into the code to see if I could fix it, which I eventually could. I've tested this with our codebase and all our files showed up correctly again 🙌 I'm not too sure if this is the right way to fix it, so I'm open for suggestions.

First I generated the changes in the snapshot by moving the added util to addon and re-exporting it in app. Then I moved it back from addon to app and manually adapted the snapshot. So, I expect this test to pass when the issue is fixed, and otherwise it will probably require a little tweak to get it working. Edit: it did require a small tweak, as I was missing the two re-exports that had no effect on the coverage in the test. The rest of the initial failing test was working as I expected it to.

If I understand correctly, the coverage is reported for files like app-namespace/components/..., in-repo-addon/components/..., paths that are then converted into app/components and lib/in-repo-addon/addon/components respectively (assuming that the in-repo addon is in lib/in-repo-addon). For the app files in those addons though, the initial path is also app-namespace/components/..., so then there is no trivial mapping to the addon's app directory possible. In the end I got it working by checking if the file exists in the app-namespace's app, and if not, it has to come from an addon. It can still come from another addon though, so then I checked per in-repo addon whether the file exists there or not. If it does exist, we know that the path should be changed to the addon's path.

This approach does work for our codebase and in the tests and I also found something else it covers now as well. In the in-repo-addon test the my-app-with-in-repo-addon/lib/my-in-repo-addon/app/services/my-service.js file isn't covered because of app/services/my-service.js, so that's a nice bonus I'd say :).

@robinborst95 robinborst95 changed the title Add failing test for files in app directory of in-repo addons Fix no coverage reported for files in app directory of in-repo addons Nov 18, 2021
@robinborst95
Copy link
Contributor Author

@kategengler @rwjblue I haven't added support for in-repo addons with a path like app-namespace/path/to/inrepo yet, but could any of you have a look at the current approach before I put more effort into finishing this 🙏 ?

@robinborst95
Copy link
Contributor Author

@thoov I see that this PR might help resolve the issue I've tried to fix here, is that what you meant it for? If it indeed does fix our issues, then I do think that it's not trivial that such a customization is needed when it can be detected by this addon as well (which was what I was striving for). Besides, if the app also defines a certain file (like I mentioned in the description of this PR with services/my-service.js), then that would fail.

@kategengler @rwjblue any thoughts on this? @rmachielse and I (and the other developers in our team) are eager to start using the beta version to speed up our development process, but this issue is a blocker for us, so any help to get this issue fixed would be appreciated 🙏

@rwjblue
Copy link
Collaborator

rwjblue commented Nov 29, 2021

ya I think that #343 is aiming to broadly address the underlying issue here

@thoov - Can you take a look?

@robinborst95
Copy link
Contributor Author

@thoov thanks for the modifyAssetLocation addition, I can confirm this works by incorporating the fix in the failing test that I added in this PR. Unfortunately, this doesn't work in our codebase yet (see #344), would there be an easy way to fix this?

@robinborst95
Copy link
Contributor Author

Closing this issue as it can be fixed with modifyAssetLocation as added in #343. Issue wasn't fixed yet, but that's a separate issue

@robinborst95 robinborst95 deleted the fix/in-repo-addon-app branch January 19, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments