Add support for gjs/gts files #405

vstefanovic97 · 2024-01-29T15:39:28Z

Disclaimer: Disclaimer this is just to make the js/ts part of the code have coverage, it does not support covering hbs part of the code. this is done in #319

So to sum it up the steps in this PR are:

Add support for inputSourceMaps (remap coverage using https://github.com/istanbuljs/istanbuljs/tree/master/packages/istanbul-lib-source-maps)

When working with already transpiled code, that has sourcemaps included (inline or in separate file) in order for the lines to be correct we actually need to use istanbul-lib-source-maps to remap the coverage to the correct source code.

This is actually needed since when we build v2 addons we already transpile the code from gjs/gts to ts/js first and we run babel on the already transpiled code, so we need to remap the coverage to the original source code.
Disclaimer: gts/gjs->js/ts transpilation doesn't generate sourcemaps at the moment but there is a PR open which will enable it embroider-build/content-tag#62

Make sure to ignore template's eval method from the coverage report

Because we instrument already transpiled code we end up instrumenting some method that shouldn't be instrumented, like for example

const var baz = <template>
  <button {{on "click" bar}}>
   increment
  </button>
</template>

gets instrumented to:

var baz = template(`
  <button {{on "click" bar}}>
   increment
  </button>
`, {
  eval() {
    cov_1sbgmdzd2v().f[2]++; // we don't want istanbul to track this
    cov_1sbgmdzd2v().s[5]++; // we don't want istanbul to track this
    return eval(arguments[0]);
  }
});

Because istanbul instruments the eval method it will report as uncovered in tests and also it skews coverage percentages, so we want it to not instrument this, for this I created a small babel plugin that will add istanbul ignore lines and run before istanbuls babel plugin.

Include .gjs/.gts extensions in istanbul extension option by default so instrumentation will work on these files
Write tests (create a v2 addon with gjs/gts where we build it with coverage and then tests the coverage with snapshots)
Lines in the snapshots coverage will still be offset until Add js/rust bindings for inline source map generation embroider-build/content-tag#62 is fixed and
we update the test app with the change

vstefanovic97 · 2024-01-30T09:17:35Z

@kategengler seems like the error reported in CI was a known PNPM issue pnpm/pnpm#5056 which was fixed with the latest pnpm version. I updated the version for CI and Volta do you mind to rerun the workflow and see if it fixes it?

vstefanovic97 · 2024-02-07T11:26:56Z

@RobbieTheWagner can you or maybe some other maintainer please review this?

RobbieTheWagner · 2024-02-07T13:24:44Z

test-packages/my-embroider-app-test.mjs

@@ -22,6 +22,7 @@ describe('app coverage generation', function () {
        "@embroider/webpack": embroiderVersion
      });

+      await execa('rm',  ['-rf', '.embroider'], { cwd: buildPath, env });


Can you please explain why this was needed?

@RobbieTheWagner so currently we are testing against emroider/core versions from 0 to 3.
During each build it creates a .embroider folder, but seems like when we run a test for a newer version it will try to re-use the .embrodier folder generated by the previous test and it was leading to some unpredictability.

For example there is a problem with babel plugin serialization in v3 that I solved here using this as a reference, but it would still fail on tests w/o cleaning up .embroider because it was relying on what was generated by the previous embroider version

So basically all I did was make sure the tests are totally isolated from each other and don't affect each other

@vstefanovic97 that's good knowledge! Mind to drop a comment in the code with this explanation for future readers?

Added a comment in the code about this @SergeAstapov, @RobbieTheWagner

vstefanovic97 · 2024-02-17T19:39:19Z

@RobbieTheWagner sorry to bother you again, any progress with the review?
Is there anything I can do to help make the reviewing easier, or anything additional that need explaining, any concerns etc. ?

RobbieTheWagner · 2024-02-17T19:42:20Z

@vstefanovic97 I really don't have enough knowledge about how this addon works to review. @SergeAstapov do you feel comfortable reviewing?

SergeAstapov

@RobbieTheWagner Thank you for pinging me!
Functionally, this looks great to me! I'd suggest we spend a bit more effort on documentation part to make clear for ppl who'd like to start using this and for ppl who would need to understand code in future

.github/workflows/ci.yml

packages/ember-cli-code-coverage/index.js

SergeAstapov · 2024-02-20T03:42:15Z

package.json

@@ -8,6 +8,7 @@
    "test": "vitest"
  },
  "devDependencies": {
+    "@babel/core": "^7.23.9",


@vstefanovic97 why is this needed in the root of monorepo? I guess each package that needs it should declare such kind of dependency.

It's used in tests here

ah, now I see - test-packages folder contains both vite tests and test packages.
IMO seems confusing but for sure we should follow existing structure in this PR.

packages/ember-cli-code-coverage/lib/attach-middleware.js

SergeAstapov · 2024-02-20T03:46:14Z

packages/ember-cli-code-coverage/lib/gjs-gts-istanbul-ignore-template-plugin.js

+            return;
+          }
+
+          path.traverse(gjsGtsTemplateIgnoreVisitor, state); // we need to do early traverse to make sure this runs before istanbuls plugin


Suggested change

path.traverse(gjsGtsTemplateIgnoreVisitor, state); // we need to do early traverse to make sure this runs before istanbuls plugin

// we need to do early traverse to make sure this runs before istanbuls plugin

path.traverse(gjsGtsTemplateIgnoreVisitor, state);

just a tiny bit more readable

Fixed @SergeAstapov

packages/ember-cli-code-coverage/lib/gjs-gts-istanbul-ignore-template-plugin.js

SergeAstapov · 2024-02-20T03:51:24Z

test-packages/my-v2-addon-gjs-gts/addon/babel.config.cjs

+      },
+    ],
+    ...require('ember-cli-code-coverage').buildBabelPlugin({
+      extension: ['.gjs', '.gts', '.js', '.ts', '.mts', '.cts', '.cjs', '.mjs'],


@vstefanovic97 this matches the default value so we can probably skip this?

SergeAstapov · 2024-02-20T03:53:03Z

test-packages/my-v2-addon-gjs-gts/addon/rollup.config.mjs

+    addon.hbs(),
+
+    // Ensure that .gjs files are properly integrated as Javascript
+    addon.gjs({ inline_source_map: true }),


@vstefanovic97 looks like it's worth to document this option in readme.
Maybe, I'm thinking we need a section with steps how to setup test coverage in v2 addons.

@SergeAstapov I agree with you on this one, but I thinking documantation for setting up coverage in a v2 addon can be a separate PR as it's not tied to gjs/gts. I will rise a new PR for that

@vstefanovic97 yeah, that's fair

vstefanovic97 · 2024-02-20T15:48:15Z

@RobbieTheWagner mind to rerun the workflow?

vstefanovic97 · 2024-02-20T16:27:10Z

@RobbieTheWagner looks like I had deleted something by accident, I fixed it now, mind running again? This time I think it will pass.

SergeAstapov · 2024-02-21T03:32:22Z

README.md

@@ -154,6 +154,8 @@ Configuration is optional. It should be put in a file at `config/coverage.js` (`

 - `excludes`: Defaults to `['*/mirage/**/*']`. An array of globs to exclude from instrumentation. Useful to exclude files from coverage statistics.

+- `extension`: Defaults to `['.gjs', '.gts', '.js', '.ts', '.cjs', '.mjs', '.mts', '.cts']`. Tell Istanbul to instrument only files with the provided extensions.


@vstefanovic97 should this actually be

Suggested change

- `extension`: Defaults to `['.gjs', '.gts', '.js', '.ts', '.cjs', '.mjs', '.mts', '.cts']`. Tell Istanbul to instrument only files with the provided extensions.

- `extensions`: Defaults to `['.gjs', '.gts', '.js', '.ts', '.cjs', '.mjs', '.mts', '.cts']`. Tells Istanbul to instrument only files with the provided extensions.

@SergeAstapov well in the code it really is extension reason for this is I decided to keep it the same as the option in Isntabul since it too is singular

test-packages/gjs-gts-ignore-template-plugin-test.mjs

SergeAstapov

if you ask me, this looks great! Thank you @vstefanovic97 for working on this! cc @RobbieTheWagner @kategengler

RobbieTheWagner

If @SergeAstapov says this looks good, that is good enough for me 👍

Update pnpm version to fix invalid string error Fix emroider comparibility bump @embroider/addon-dev and assert sorcemap remap is working Add a comment why we need to cleanup .embroider folder between test runs Improve docs and add more comment explanations remove explicit extensions from test app bring back babel transpilation plugin Fix type Co-authored-by: Sergey Astapov <SergeAstapov@gmail.com> Fix typo 2 Co-authored-by: Sergey Astapov <SergeAstapov@gmail.com>

vstefanovic97 · 2024-02-21T14:58:50Z

@RobbieTheWagner mind to run again (I think this will be the final time)? I changed test name but forgot to update the snapshot

vstefanovic97 · 2024-02-21T20:27:29Z

@RobbieTheWagner, @SergeAstapov can we merge this and create a new minor release?

SergeAstapov · 2024-02-21T21:20:59Z

IMO yes, we are good to go, but I don't have permissions here.
@RobbieTheWagner would you be able to push this over the line and do the release?

cah-brian-gantzler · 2024-02-23T17:14:54Z

After updating to this version I am now getting no coverage directory being created at all. Anything else I might have to do to configure? I did already have all the configurations from the readme in place for the prior version.

vstefanovic97 · 2024-02-23T17:39:48Z

@cah-brian-gantzler there should be no additional config required for it to work.

Were you on 2.0.3 before this?
Do you see any Errors or something?
Is it a regular Ember app/addon, is it Embroider App or v2 addon?

Also if you can come up with a minimal reproduction I could also try to debug it

cah-brian-gantzler · 2024-02-23T18:13:41Z

was on 2.0.3
No do not see any errors.
it is an embroider app

Since its in my app, doubt and will prob work with a new app, not sure where to start on a minimal not knowing what the problem is.

Will experiment some. Its weird that it produces NO coverage at all though. Will also revert back and make sure it still produces then. Did last time I asked, but that was a couple weeks ago, was waiting for this update

cah-brian-gantzler · 2024-02-24T16:51:06Z

@vstefanovic97 must have been a glitch. Now produces COVERAGE directory, however no .gjs or .gts files appears appear (same as before)

I checked to ensure I had the correct package in my node_modules, and the package.json says 2.1.1 However noticed that the changelog.md is out of date. You might want to consider copying the changelog.md from the root to the package before publishing.

I found I also picked up useBabelInstrumenter: true, in the config file over the years. Not sure where I got that, but removing it did not alter the output.

Lastly I have my mirage directory in the app folder and not the root. Altering the excludes to */app/mirage/**/* or app/mirage/**/* did not remove mirage from the report, but **/mirage/**/* did although that seems overkill. Got a suggestion on what the glob should be? Not great at globs

vstefanovic97 · 2024-02-24T17:05:48Z

@cah-brian-gantzler can you build the app with COVERAGE=true it can be either a production or development build, and check if the code for your gjs/gts files is instrumented with Istanbul?

Here is an example of what istanbul should do to the code, if you aren't familiar
https://gist.github.com/robertknight/834452c3b06963ff2a8b9682fd4189cb

Maybe also it's worth checking if you have source maps turned on for your tests
https://github.com/ember-template-imports/ember-template-imports?tab=readme-ov-file#sourcemap-generation
This part is actually needed for the lines to be accurate.

Let's first check this then we can narrow it down further.
I don't have time today, but I'll try this out on a new embroider app to see if it works.

Also I got some feedback here that it works #392 (comment) so maybe what you are seeing is some edge case, but we will try to figure it out and fix :D

cah-brian-gantzler · 2024-02-24T19:16:19Z

@vstefanovic97 Thanks, I do know what instrumented code looks like.

I did the build, but couldnt really find the code in the asset chunks, no sure how to do that. I put a debugger statement in the code in the .gts and ran the tests, the code was definitely instrumented. No change on the output in coverage.

I do not have the source maps turned on for template imports. The read me says it might be useful, not required. If it is required to get correct line numbers, then I would say the docs should be updated to say required. Also says should not be enabled for production. Should prob give example on how to do that, most people wouldnt know.

End result though. Code is instrumented, source maps now turned on for template imports (as above), no change in output. Any file that has a .gjs or .gts does not appear in code coverage.

On a side note, for a long time the code that is in the coverage has always been off by one line. It still is.

vstefanovic97 · 2024-02-27T00:24:18Z

@cah-brian-gantzler this looks to be an embroider + ember-cli-code-coverage with gjs/gts issue

I was able to reproduce with a band new embroider app.
I am still trying to dig into the root cause and give it a proper fix

For now can you try to add

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;
  },
};

to your config/coverage.js file
I think this will get the files to show, but for some reason coverage percentage is still not accurate (I'm still debugging this).
Btw do you mind opening an issue for this, it will be simpler to track there and not in this PR as it's closed

NullVoxPopuli · 2024-02-28T16:15:10Z

packages/ember-cli-code-coverage/index.js

@@ -72,7 +88,13 @@ module.exports = {
    }

    const IstanbulPlugin = require.resolve('babel-plugin-istanbul');
-    return [[IstanbulPlugin, { cwd, include: '**/*', exclude }]];
+    return [
+      // String lookup is needed to workaround https://github.com/embroider-build/embroider/issues/1525


As it turns out... We're going to require string lookup eventually, and forbid passing JS references.

JS references are not parallizable, but string references are! (it's a babel thing 🤷 )

hey @NullVoxPopuli are there any problems or situations where this string lookup might not work?
I just tried this out on a new embroider app, I'm investigating #407 and I don't get any errors but it seems like that plugin isn't running

one thing you could try is using a resolved string, so you get the absolute path of the plugin for "this instance of ember-cli-code-coverage".

path.resolve('the-string-you-have'); // returns absolute string, so no matter who is requiring, their resolve isn't local to who is doing the requiring

vstefanovic97 mentioned this pull request Jan 29, 2024

Issues with first-class component templates? #392

Closed

vstefanovic97 force-pushed the master branch from c15a900 to 947c94a Compare February 6, 2024 12:53

RobbieTheWagner reviewed Feb 7, 2024

View reviewed changes

SergeAstapov suggested changes Feb 20, 2024

View reviewed changes

SergeAstapov reviewed Feb 21, 2024

View reviewed changes

vstefanovic97 force-pushed the master branch from c08c8c7 to 564962e Compare February 21, 2024 12:48

SergeAstapov approved these changes Feb 21, 2024

View reviewed changes

RobbieTheWagner approved these changes Feb 21, 2024

View reviewed changes

RobbieTheWagner added the enhancement label Feb 21, 2024

vstefanovic97 force-pushed the master branch from 564962e to c568ae9 Compare February 21, 2024 14:57

SergeAstapov approved these changes Feb 21, 2024

View reviewed changes

RobbieTheWagner changed the title ~~Add coverage logic for gjs/gts files~~ Add support for gjs/gts files Feb 22, 2024

RobbieTheWagner merged commit 32ef303 into ember-cli-code-coverage:master Feb 22, 2024
1 check passed

NullVoxPopuli reviewed Feb 28, 2024

View reviewed changes

	path.traverse(gjsGtsTemplateIgnoreVisitor, state); // we need to do early traverse to make sure this runs before istanbuls plugin
	// we need to do early traverse to make sure this runs before istanbuls plugin
	path.traverse(gjsGtsTemplateIgnoreVisitor, state);

		@@ -154,6 +154,8 @@ Configuration is optional. It should be put in a file at `config/coverage.js` (`

		- `excludes`: Defaults to `['/mirage//']`. An array of globs to exclude from instrumentation. Useful to exclude files from coverage statistics.

		- `extension`: Defaults to `['.gjs', '.gts', '.js', '.ts', '.cjs', '.mjs', '.mts', '.cts']`. Tell Istanbul to instrument only files with the provided extensions.

	- `extension`: Defaults to `['.gjs', '.gts', '.js', '.ts', '.cjs', '.mjs', '.mts', '.cts']`. Tell Istanbul to instrument only files with the provided extensions.
	- `extensions`: Defaults to `['.gjs', '.gts', '.js', '.ts', '.cjs', '.mjs', '.mts', '.cts']`. Tells Istanbul to instrument only files with the provided extensions.

Add support for gjs/gts files #405

Add support for gjs/gts files #405

Conversation

vstefanovic97 commented Jan 29, 2024 • edited

vstefanovic97 commented Jan 30, 2024

vstefanovic97 commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstefanovic97 commented Feb 17, 2024

RobbieTheWagner commented Feb 17, 2024

SergeAstapov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstefanovic97 commented Feb 20, 2024

vstefanovic97 commented Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeAstapov left a comment

Choose a reason for hiding this comment

RobbieTheWagner left a comment

Choose a reason for hiding this comment

vstefanovic97 commented Feb 21, 2024

vstefanovic97 commented Feb 21, 2024

SergeAstapov commented Feb 21, 2024

cah-brian-gantzler commented Feb 23, 2024

vstefanovic97 commented Feb 23, 2024 • edited

cah-brian-gantzler commented Feb 23, 2024

cah-brian-gantzler commented Feb 24, 2024

vstefanovic97 commented Feb 24, 2024

cah-brian-gantzler commented Feb 24, 2024 • edited

vstefanovic97 commented Feb 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstefanovic97 commented Jan 29, 2024 •

edited

vstefanovic97 commented Feb 20, 2024 •

edited

vstefanovic97 commented Feb 23, 2024 •

edited

cah-brian-gantzler commented Feb 24, 2024 •

edited

vstefanovic97 commented Feb 27, 2024 •

edited