Skip to content
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

Coverage for gjs/gts files do not work for embroider apps #407

Closed
cah-brian-gantzler opened this issue Feb 27, 2024 · 17 comments
Closed

Coverage for gjs/gts files do not work for embroider apps #407

cah-brian-gantzler opened this issue Feb 27, 2024 · 17 comments
Labels

Comments

@cah-brian-gantzler
Copy link

Upgraded to 2.1.1, the gjs/gts files do not appear in the coverage output.

This seems to be an issue with embroider.

@vstefanovic97 was able to reproduce the issue. Please update any more information here.

@cah-brian-gantzler
Copy link
Author

cah-brian-gantzler commented Feb 27, 2024

More questions that answers.

I only get test coverage when -s is added to the ember t. Without it, no coverage is written. (See #403 (comment))

Adding your method of

const path = require('path');

module.exports = {
  modifyAssetLocation(root, relativePath) {
    if (relativePath.includes('/app-name/')) {
      return path.join('app', relativePath.split('/app-name/')[1]);
    }

    return false;
  },
};

Does seem to work. I get the gjs/gts files showing. But I dont know why because adding a console.log(relativePath); as the first statement shows that all the relative paths passed do not have the app name in them.

components/invoice-master-listing.js
components/invoice-master-status.js
components/invoice-master-view.js
components/invoice-z120-export.js
components/invoice/admin/credit-review-item.js
components/invoice/admin/invoice-report.js
components/invoice/admin/open-cycles.js
components/invoice/admin/open-disputes-and-deferreds-item.js
components/invoice/admin/open-z120-invoice-review-item.js
components/invoice/administrator-invoice-edit.js

Which means the method should only be returning false all the time. However if I comment everything out and only return false all the time, it doesnt work. EDIT: Moving the console.log into the if prints the console all the time. Not sure why but the if is always true.

gjs/gts files also seem to be correct on lines. All js/hbs versions are off by one line (have been for a long time for me)

@RobbieTheWagner
Copy link
Collaborator

Thanks for digging in @cah-brian-gantzler! @vstefanovic97 do you know what might be going on here?

@vstefanovic97
Copy link
Contributor

@RobbieTheWagner it has something to do with how Embroider is copying over some files in some tmp dir, I didn't have time to fully investigate and come up with a proper solution due to work, but hope to get back to this soon

@bgantzler
Copy link

I see some people in discord where also reporting that coverage only works when -s is applied.

On a brand new app, it appears to work without the -s. Not sure why. On my full app it does not.

Brand new app with embroider though is missing coverage for gjs/gts still. Will try adding the modifyAsset code from above

@vstefanovic97
Copy link
Contributor

@RobbieTheWagner, @cah-brian-gantzler I think I've identified several issues and fixes, working on adding better test cases for future.
Hope to push the fixes soon.
Btw @RobbieTheWagner are you maybe able to give me permissions just to run CI by myself so I don't need to ping you every time tests fail on CI?

@cah-brian-gantzler
Copy link
Author

Great to hear. Looking forward to it. Appreciate your efforts.

@SergeAstapov
Copy link
Contributor

@vstefanovic97 you won’t need to ask for that from now on as you already contributed to this repo so CI will kick off automatically

@vstefanovic97
Copy link
Contributor

vstefanovic97 commented Mar 12, 2024

@cah-brian-gantzler, @RobbieTheWagner I just made a PR
#408
That I hope should resolve most of the issues.
It fixes:

  • App with throwUnlessParallelizable: true (ember-cli-babel)
  • Classic app with ember-template-imports (added tests for this)
  • Embroider app with ember-template-imports (wasn't able to add test case but verified on my local) (added tests for this)

@RobbieTheWagner
Copy link
Collaborator

Can we close this now?

@SergeAstapov
Copy link
Contributor

SergeAstapov commented Mar 25, 2024

@cah-brian-gantzler we would appreciate if you could confirm v2.1.2 fixes issues reported here (except ember s scenario as it's separate)

@cah-brian-gantzler
Copy link
Author

Will do that sometime today

@cah-brian-gantzler
Copy link
Author

I have confirmed updating to 2.1.2 producing the expected results for gjs/gts when using COVERAGE=true ember t -s.

Also want to note that the lines marked in the HTML are the correct lines. In the .js files they are off by one line.

@RobbieTheWagner
Copy link
Collaborator

I have confirmed updating to 2.1.2 producing the expected results for gjs/gts when using COVERAGE=true ember t -s.

Also want to note that the lines marked in the HTML are the correct lines. In the .js files they are off by one line.

Is there something we need to do to make them the right lines?

@cah-brian-gantzler
Copy link
Author

cah-brian-gantzler commented Mar 26, 2024

I hadnt noticed before but its only components, all other (controllers, routes, etc) are on the correct lines. It seems only components are off by one line (js components only. The gjs/gts versions are on the correct lines)

image

@SergeAstapov
Copy link
Contributor

@cah-brian-gantzler just to confirm, this 1 line off issue is present only for components in .js format and not present for .gts/.gjs?

@cah-brian-gantzler
Copy link
Author

That is correct. only .js. Here is a snippet from a gts file (you can see the </template> at the top.

Interesting, I am assuming that services (line 21) are not marked as accessed even though they are referenced (line 36). Makes sense, how would you know. Yet tracked variables (line 30) are. Are the 2 times (line 30) sets I wonder, since gets were at least 5 (line 37). Must be why the service isnt marked, Variables must be only marked as sets only.

image

@cah-brian-gantzler
Copy link
Author

This issue, as is, can be closed. Will open another issue for the off by one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants