Enable coverage reporting #3100

michaelfig · 2021-05-14T18:49:26Z

See COVERAGE.md for more information.

github-actions · 2021-05-14T19:28:22Z

🚀 Deployed on https://60a077c926ff1fbf88ab9aab--agoric-sdk-coverage.netlify.app

michaelfig · 2021-05-16T00:58:02Z

We now break the deadlock as described in #527 (comment) by using standard-things/esm#877

soil:t michael$ ls
main.cjs                module.js               package.json
soil:t michael$ cat main.cjs 
require = require("esm")(module);
const m = require("./module.js");
console.log(m.X);
soil:t michael$ cat module.js 
console.log('in module');
export const X = 1;
soil:t michael$ node module.js
in module
soil:t michael$ node main.cjs
/Users/michael/agoric/agoric-sdk/packages/t/module.js:1
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/michael/agoric/agoric-sdk/packages/t/module.js
require() of ES modules is not supported.
require() of /Users/michael/agoric/agoric-sdk/packages/t/module.js from /Users/michael/agoric/agoric-sdk/packages/t/main.cjs is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename module.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/michael/agoric/agoric-sdk/packages/t/package.json.

    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1080:13) {
  code: 'ERR_REQUIRE_ESM'
}

After the ESM patch in this PR:

soil:t michael$ node main.cjs
in module
1

patches/esm+3.2.25.patch

michaelfig · 2021-05-16T02:40:39Z

This is about as good as coverage is going to get until the caveats in COVERAGE.md are lifted.

.github/workflows/test-all-packages.yml

warner · 2021-05-17T07:00:35Z

COVERAGE.md

+well as implementing source maps in all of our source-to-source transforms (such
+as `@agoric/bundle-source`, `@agoric/transform-metering`, and
+`@agoric/static-module-record`), the coverage line numbers will be out-of-sync
+with reality.


That's a pretty big caveat. Do the percentages still seem to be accurate? Is there a human-manageable way to get from the line numbers this emits to the actual source code? Or should we preface all the reports with a note that we should ignore the line numbers entirely and just look at the percentages?

Also, does this affect basic unit tests that use -r esm (like everything we do) but do not use bundle-source or import-bundle? I'd be happy with getting accurate coverage for library code that we can test outside the context of swingset.

So the basic plan is: if we adopt the Node.js esm package patch I made in this PR, we can do a gradual bottom-up migration of our packages to be dual -r esm and Node.js ESM (nesm) compatible.

I've changed the HTML file generation only to coverage-test the packages which can support nesm and explicitly implement a test:c8 script. Currently only swing-store-simple implements that (and incidentally, also supports resm).

warner · 2021-05-17T07:04:58Z

package.json

@@ -82,6 +82,15 @@
    "patch-package": "patch-package",
    "build-xs-worker": "cd packages/xs-vat-worker && yarn build:xs-lin"
  },
+  "ava": {
+    "files": [
+      "packages/*/test/**/test-*.js"


Does this interfere with / override ava settings in individual package's yarn test?

It looks like this is meant for use by a top-level yarn test which runs tests in all packages in parallel. When I last looked into doing that, I got stuck on the different require options we needed for different sets of tests (in particular importing install-ses, or install-metering-and-ses). Did you find a solution for that?

Does this interfere with / override ava settings in individual package's yarn test?

Only when run from the top level. If yarn test is run in the specific package, the package's settings will take effect.

It looks like this is meant for use by a top-level yarn test which runs tests in all packages in parallel. When I last looked into doing that, I got stuck on the different require options we needed for different sets of tests (in particular importing install-ses, or install-metering-and-ses). Did you find a solution for that?

Yes. It turns out we only use "require" for two modules: "esm" (which appears in basically every "ava" config), and an enzyme setup script (only used in packages/ui-components/package.json). So, the situation is not nearly as grim.

Ok, I think @dtribble was once keen on using the AVA config to jam a -r @agoric/install-ses into e.g. all zoe tests, to avoid adding a line to the top of every test file, and that was going to push us to e.g. move all @agoric/install-metering-and-ses tests into one directory, and have two top-level AVA calls (with one set of -r options each). I think we talked him down from that :).

I've got nodeArguments: [ "--expose-gc" ] in the SwingSet package.json, to enable GC controls while testing. We may need to move that to the top-level package.json to make sure swingset tests work both from the top level and from packages/SwingSet (and also when e.g. Zoe does swingset-based tests, although if they're short enough, we can probably tolerate the inability to provoke GC).

warner · 2021-05-17T07:07:08Z

packages/swing-store-simple/package.json

@@ -5,14 +5,14 @@
  "parsers": {
    "js": "mjs"
  },
+  "type": "module",


Ooh, does this work? I'm suspicious that this PR changes the type for this one package and none of the others. @kriskowal does this interact well/badly with the other ESM/-r ESM changes you're planning?

Does #527 (comment) answer these questions?

Ooh, does this work?

Yep!

I'm suspicious that this PR changes the type for this one package and none of the others.

I did that to demonstrate that incremental, bottom-up support of NESM is possible without having to throw away -r esm.

@kriskowal does this interact well/badly with the other ESM/-r ESM changes you're planning?

In talking with @kriskowal, this is a fine step forward.

warner · 2021-05-17T07:08:53Z

patches/esm+3.2.25.patch

@@ -1,9 +1,9 @@
 diff --git a/node_modules/esm/esm/loader.js b/node_modules/esm/esm/loader.js
-index e0bbca5..8710025 100644
+index e0bbca5..0b5bfe8 100644


I'm surprised this patch changed, since we didn't change the version of esm that we're using. Is this intentional? Same for the other patch.

I'm surprised this patch changed, since we didn't change the version of esm that we're using. Is this intentional?

Yes, as discussed in #527 (comment)

Same for the other patch.

nyc is no longer referenced, so I needed to remove that patch or patch-package complains about its absence.

katelynsills

The changes to Zoe and ERTP look fine to me. I defer to whatever @warner and @dckc think, but let me know if there's anything you want my eye on.

kriskowal

It might be good to also change scripts/repackage.sh to reflect this new mode.

kriskowal · 2021-05-17T21:30:54Z

COVERAGE.md

+open coverage/html/index.html
+```
+
+## NESM Support


Let’s spell this “Node.js ESM” formally.

kriskowal · 2021-05-17T21:55:58Z

How is your fork of esm linked in this? I see nothing changed with the package.json name. Are we publishing as esm as if upstream.

michaelfig · 2021-05-17T22:04:00Z

How is your fork of esm linked in this? I see nothing changed with the package.json name. Are we publishing as esm as if upstream.

We're patching upstream esm with agoric-sdk/patches/esm+3.2.25.diff

.github/workflows/test-all-packages.yml

COVERAGE.md

warner

Ok, as long the top-level yarn test provides the right set of AVA options to the right packages (I'm specifically looking at nodeArguments: [ "--expose-gc" ] for SwingSet, as I'm about to add a test that depends upon it), I'm in.

I added this to SwingSet/package.json in commit d4bc617 (PR #3136). However I overlooked the fact that #3100 had already landed, which changed the top-level `yarn test` from one that just did a (sequential) `yarn workspaces test`, testing one package at a time with whatever the local `package.json` said to do, to one that does a (parallel) `ava` invocation, which does all packages at the same time but ignores the local `package.json` settings. We need to include `--expose-gc` in the `nodeArguments` in the top-level package.json to make sure the parallel form give the correct arguments when SwingSet's turn comes around (as well as other packages which benefit from enabling GC but don't have tests which directly assert that it can be done).

michaelfig self-assigned this May 14, 2021

michaelfig added the tooling repo-wide infrastructure label May 14, 2021

michaelfig force-pushed the mfig-enable-coverage branch from f243786 to 80024ca Compare May 14, 2021 18:53

michaelfig mentioned this pull request May 16, 2021

use Node's ES-module support, stop using -r esm #527

Closed

michaelfig requested review from katelynsills and warner May 16, 2021 01:07

michaelfig marked this pull request as ready for review May 16, 2021 01:08

michaelfig force-pushed the mfig-enable-coverage branch from 0eb1573 to bbd7554 Compare May 16, 2021 01:10

dckc reviewed May 16, 2021

View reviewed changes

patches/esm+3.2.25.patch Show resolved Hide resolved

warner reviewed May 17, 2021

View reviewed changes

.github/workflows/test-all-packages.yml Outdated Show resolved Hide resolved

warner reviewed May 17, 2021

View reviewed changes

michaelfig force-pushed the mfig-enable-coverage branch from 50a0c92 to 9f83426 Compare May 17, 2021 20:43

katelynsills reviewed May 17, 2021

View reviewed changes

kriskowal approved these changes May 17, 2021

View reviewed changes

michaelfig added 8 commits May 19, 2021 19:29

chore: rip out per-package nyc support

b82d545

feat: make top-level yarn c8 ava work

89ac4c7

ci: build and deploy https://agoric-sdk-coverage.netlify.app

d58f234

fix: patch ESM to allow bottom-up implementation of type: "module"

d3b2549

docs: describe limitations of c8 under source transforms

5877439

fix: convert swing-store-simple to "type": "module"

93279c1

ci: remove node 12.14.1 as it doesn't support "type":"module"

6d1b9c7

ci: only generate coverage for NESM-supporting packages

3192156

michaelfig force-pushed the mfig-enable-coverage branch from 77d71db to fe3ef2d Compare May 20, 2021 01:30

refactor: update COVERAGE.md to accommodate ava-nesm.config.js

856abbe

michaelfig force-pushed the mfig-enable-coverage branch from fe3ef2d to 856abbe Compare May 20, 2021 01:32

warner reviewed May 20, 2021

View reviewed changes

.github/workflows/test-all-packages.yml Show resolved Hide resolved

warner reviewed May 20, 2021

View reviewed changes

COVERAGE.md Outdated Show resolved Hide resolved

warner approved these changes May 20, 2021

View reviewed changes

ci: clean up to be ready for master

464b464

michaelfig force-pushed the mfig-enable-coverage branch from 1a7f906 to 464b464 Compare May 20, 2021 02:12

michaelfig enabled auto-merge May 20, 2021 02:12

michaelfig merged commit 0cae5d7 into master May 20, 2021

michaelfig deleted the mfig-enable-coverage branch May 20, 2021 02:26

warner mentioned this pull request May 24, 2021

fix: use --expose-gc for top-level 'yarn test' #3159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable coverage reporting #3100

Enable coverage reporting #3100

michaelfig commented May 14, 2021

github-actions bot commented May 14, 2021 •

edited

Loading

michaelfig commented May 16, 2021 •

edited

Loading

michaelfig commented May 16, 2021

warner May 17, 2021

warner May 17, 2021

michaelfig May 17, 2021

warner May 17, 2021

michaelfig May 17, 2021

warner May 20, 2021

warner May 17, 2021

dckc May 17, 2021

michaelfig May 17, 2021

warner May 17, 2021

michaelfig May 17, 2021

katelynsills left a comment

kriskowal left a comment

kriskowal May 17, 2021

michaelfig May 20, 2021

kriskowal commented May 17, 2021

michaelfig commented May 17, 2021

warner left a comment

Enable coverage reporting #3100

Enable coverage reporting #3100

Conversation

michaelfig commented May 14, 2021

github-actions bot commented May 14, 2021 • edited Loading

michaelfig commented May 16, 2021 • edited Loading

michaelfig commented May 16, 2021

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

Choose a reason for hiding this comment

katelynsills left a comment

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented May 17, 2021

michaelfig commented May 17, 2021

warner left a comment

Choose a reason for hiding this comment

github-actions bot commented May 14, 2021 •

edited

Loading

michaelfig commented May 16, 2021 •

edited

Loading