support new embroider working dir introduced in embroider/compat >3.1 #387
Conversation
chrismllr
changed the title
chrismllr
changed the title
|
Do you have a sample app that proves that this works? |
|
@NullVoxPopuli Sure thing, I can see about duping the repro repo and applying these changes. |
|
LEFT screenshot: Test run from reproduction repo (no coverage output): https://github.com/chrismllr/repro-code-coverage-embroider/actions/runs/5881955365/job/15951451547 RIGHT screenshot: Test run from fix repo (coverage output successfully): https://github.com/chrismllr/fix-code-coverage-embroider/actions/runs/5881976092/job/15951525068 
|
| let version = pkgJSON['devDependencies']['@embroider/compat']; | ||
| let semver = require('semver'); |
There was a problem hiding this comment.
This admittedly feels flimsy, but this.project was not available in scope here (to pass on to something like ember-cli-version-checker)
There was a problem hiding this comment.
Perhaps a try/catch for importing/requiring the new locateEmbroiderWorkingDir method
There was a problem hiding this comment.
Going to migrate to use the above, saw some issues in tests with fixturify-project usage (these values read were not the fixture libs output). This method also feels more stable. If the method exists, use it. Otherwise, use the original.
|
cc @mansona for embroider support here (unless there is someone else I should ping?) |
|
I think it may be necessary to bump the peerDep requirements on edit: other than that these changes appear to work fine for our ember-exam + coverage setup on both embroider v2 as well as latest v3. |
|
@chrismllr can you confirm if the current version is only working with embroider 1? I've tested with embroider 2 but no coverage was reported either. This is currently a blocker for me. |
|
cc: @ef4 |
|
@chrismllr I would love to get this in, but want to make sure it doesn't make things blow up. We probably at least need to guard |
|
@chrismllr @RobbieTheWagner would it worth adding a test Embroider app to ensure this addon works? |
|
@SergeAstapov there is one https://github.com/ember-cli-code-coverage/ember-cli-code-coverage/tree/master/test-packages/my-embroider-app I think we would need a couple with different embroider versions. Tests are currently failing for this embroider app, so if anyone has time to help with this, I would appreciate it! |
|
Hey all, apologies for the lack of updates here, I have been taking paternity leave 👶 I can attempt to get in to get some tests added as time permits, but if there's a way for someone to pick up and run with it, I can assist in any way I can! As for some questions:
Could you clarify? Currently, its behind a
Nothing beyond my local testing; I've got a test repo mentioned in the original issue, but this may require some tests as others have mentioned. |
|
I merged #398 so not sure if we still need this PR? |
Ah, does that PR use some other method to tell istanbul where to look for app files? |
|
@chrismllr I've only added a test for embroider/compat > 3.1. But i haven't changed anything related to this issue. |
There don't appear to be any code changes to the core of code-coverage in the other PR, so I doubt it fixes it and it'd make this PR still required. |
I've added a commit to this fork to enable the above test to run on 3.1.0, and looks like its also pulled in some pnpm-lock updates per changes made in the last merged PR. Is there anything necessary to do to test the actions to run? |
|
Currently working through the tests for 3.1, but all embroider tests for There look to be some discrepancies with how |
- if `locateEmbroiderWorkingDir` exists, use it. otherwise use prior - remove (seemingly) unnecessary update to ^3.0 peer dep - remove semver. no longer used
|
Altered the logic to use presence of the method instead of semver/ introspection of package.json. All tests passing for me locally! |
RobbieTheWagner
merged commit 494396b
into
ember-cli-code-coverage:master
Relates to #386
Allow Istanbul to observe the correct files when embroider builds into its new working directory location.
As of this commit: embroider-build/embroider@9e078f0, published in version 3.1.5 of
@embroider/compat, Embroider uses a newlocateEmbroiderWorkingDirmethod to obtain where the "rewritten" app is built to, which now resolves tonode_modules/.embroider/rewritten-app.