Wrap ava test function to log failures to actual console #555

erights · 2021-01-01T06:51:44Z

When a test case test(title, func) fails because func threw an error or returned a promise that eventually rejects, ava prints the stack in what looks like console output but is not. This bypasses all of our fancy error support --- the error details and all the deep stack support -- which made it extremely unpleasant to debug failing tests.

This PR provides a testWrapper function that acts like ava's test function, except that when a test fails, the failure is also reported to the actual console. When used under SES, this is the fancy console that SES installs.

Since this only does something when the ava test fails, there's no easy way to include an automated test. However I have monkey tested it (in Agoric/agoric-sdk#2159 ) and it seems to work.

fixes #577

erights · 2021-01-01T06:57:17Z

What else do I need to do so this is pleasantly importable by other tests not in this monorepo?

erights · 2021-01-01T07:16:54Z

On the monkey testing...

At line 739 of https://github.com/Agoric/agoric-sdk/pull/2159/checks?check_run_id=1632858610#step:7:739 we see

REJECTED from ava test: (Error#1)

  Error#1: The trade between left Object <[Object: null prototype] {}> {
    seat: Object <[Object: null prototype] {}> {
      exit: [Function: exit] Function <Function <Complex prototype>>,
      fail: [Function: fail] Function <Function <Complex prototype>>,
      ...
    },
    gains: Object <[Object: null prototype] {}> {
    ...

Error#1 ERROR_NOTE: due to (Error#2)
  Error#1 ERROR_NOTE: Thrown from: (Error#3) : 429 . 2
  Error#1
    Error#2: right element Object <[Object: null prototype] {}> {
      handle: Object <[Object: null prototype] {}> { '@pattern': '*' },
      instance: Object <[Object: null prototype] {}> { '@pattern': '*' },
      installation: Object <[Object: null prototype] {}> {
        getBundle: [Function: getBundle] Function <Function <Complex prototype>>
      },
      description: 'exerciseOption',
      underlyingAssets: Object <[Object: null prototype] {}> {
        UnderlyingAsset: Object <[Object: null prototype] {}> {
          brand: [Alleged: moola brand <Complex prototype> [Alleged: moola brand]],
          value: 3
        }

also with deep stack traces

All this is extremely valuable debugging info we weren't seeing despite all our error/console/assert engineering. Hidden by ava.

kriskowal · 2021-01-04T23:40:23Z

It would be good to see this side-by-side with intended internal usage. I don’t think we want to entrain this in the SES public API. Since this is a mitigation against Ava specifically, we would ideally package this with a name like ses-ava. We could do so here and publish it to npm to get it in agoric-sdk.

kriskowal

To answer your question regarding making this public API, we have options.

Publishing from the SES package would imply some Ava coupling I’d rather avoid.

I’d suggest creating a thing ses-ava package in ses-shim and publishing it to npm so it can be used in other projects. If we also need to use the test wrapper for our own SES tests, let’s do that in this change, using ses-ava as a development dependency.

erights · 2021-01-04T23:44:46Z

Good plan! Will do.

erights · 2021-01-12T16:56:00Z

Hi @kriskowal, When you have time, please look at this. I started with your repackage script and tried to adjust from there blindly using other packages for examples. When I yarn build I get the following. What have I misconfigured? What needs to be adjusted in repackage.sh ?

lerna ERR! npm run build exited 1 in '@agoric/ses-ava'
lerna ERR! npm run build stdout:

> @agoric/ses-ava@0.0.0 build /Users/markmiller/src/ongithub/agoric/SES-shim/packages/ses-ava
> rollup --config rollup.config.js


lerna ERR! npm run build stderr:

src/main.js → dist/ses-ava.mjs, dist/ses-ava.cjs...
[!] Error: 'default' is not exported by node_modules/ava/index.js, imported by src/ses-ava-test.js
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
src/ses-ava-test.js (1:7)
1: import test from 'ava';
          ^
2: 
3: const avaTest = test;
Error: 'default' is not exported by node_modules/ava/index.js, imported by src/ses-ava-test.js
    at error (/Users/markmiller/src/ongithub/agoric/SES-shim/packages/compartment-mapper/node_modules/rollup/dist/shared/rollup.js:5265:30)

kriskowal

I believe the error is because Rollup needs a plugin to treat CommonJS dependencies (ava) properly. Only packages that have CommonJS dependencies of Rollup artifacts require this plugin. The plugin is already in the dependencies, just was not in the template Rollup configuration.

packages/ses-ava/rollup.config.js

erights · 2021-01-12T18:27:28Z

All suggestions applied. Now yarn build failing with the following. Just in case I tried doing a yarn install --force and tried again and the yarn build still fails.

lerna ERR! npm run build exited 1 in '@agoric/ses-ava'
lerna ERR! npm run build stdout:

> @agoric/ses-ava@0.0.0 build /Users/markmiller/src/ongithub/agoric/SES-shim/packages/ses-ava
> rollup --config rollup.config.js


lerna ERR! npm run build stderr:

src/main.js → dist/ses-ava.mjs, dist/ses-ava.cjs...
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
data:text/javascript, (imported by node_modules/ava/lib/worker/subprocess.js)
[!] Error: Unexpected token (Note that you need @rollup/plugin-json to import JSON files)
node_modules/ava/package.json (2:7)
1: {
2:   "name": "ava",
           ^
3:   "version": "3.15.0",
4:   "description": "Node.js test runner that lets you develop with confidence.",
Error: Unexpected token (Note that you need @rollup/plugin-json to import JSON files)
    at error (/Users/markmiller/src/ongithub/agoric/SES-shim/packages/compartment-mapper/node_modules/rollup

packages/ses-ava/src/types.js

erights · 2021-02-22T05:33:58Z

See #579

erights · 2021-03-05T05:08:06Z

The state of this PR is that the new source files and test files are all working and ready for review. However...

The first commit is the pristine state resulting from @kriskowal 's scripts/repackage.sh. Every change I've made to every one of the files it created, especially package.json, I did by cargo culting patterns I saw in other projects without much or any understanding. All those changes should be viewed with great suspicion and are probably wrong. Feel free to replace them in their entirety.

Since the point of this PR is to change what happen when ava reports failure, we can't code this in ava directly as passing tests. To do this correctly, we need #579 which I don't know how to do. Instead, the substantive tests are all commented out of that this package passes CI. If you're inclined to fix this by doing #579 first, that would be great!

@dckc I'm out tomorrow (Friday) but will be back Saturday. Please proceed towards making all these improvements. @kriskowal please help @dckc , especially with the changes I made to the files left behind by scripts/repackage.sh.

Thanks!

erights · 2021-03-05T05:55:47Z

See Agoric/agoric-sdk#2582

`@callback` doesn't seem to mix with spread syntax, so we use `@typedef` instead for Logger.

dckc

I went ahead and fixed any -> unknown.

My other comments are not critical.

dckc · 2021-03-05T15:43:57Z

packages/ses-ava/package.json

+    "lint-fix": "eslint --fix '**/*.js'",
+    "prepublish": "yarn clean && yarn build",
+    "test": "yarn build && yarn ava",
+    "qt": "yarn ava"


Having yarn test run yarn build was surprising. I did eventually discover qt... but I ran scripts/repackage.sh skel and discovers it produces just "test": "ava".

Thoughts, @kriskowal ?

I don't suppose it's critical.

dckc · 2021-03-05T15:46:04Z

packages/ses-ava/package.json

+  "scripts": {
+    "build": "rollup --config rollup.config.js",
+    "clean": "rm -rf dist",
+    "lint": "yarn lint:types && yarn lint:js",


scripts/repackage.sh produces build:types rather than lint:types. I consider that a bug in scripts/repackage.sh.

dckc · 2021-03-05T15:47:59Z

packages/ses-ava/package.json

+    "@rollup/plugin-commonjs": "^13.0.0",
+    "@rollup/plugin-node-resolve": "^6.1.0",
+    "@rollup/plugin-json": "^4.1.0",
+    "@agoric/eslint-config": "^0.1.0",


scripts/repackage.sh seems to be out of date on eslint config too.

dckc · 2021-03-05T15:51:00Z

packages/ses-ava/jsconfig.json

+    "target": "es2020",
+    "module": "esnext",
+    "noEmit": true,
+    "downlevelIteration": true,


downlevelIteration seems to be deprecated, but I see it in lots of other jsconfig.json files, so it's pretty clearly harmless

dckc · 2021-03-05T15:54:04Z

packages/ses-ava/src/ses-ava-test.js

+/**
+ * Just forwards to global `console.error`.
+ *
+ * @type {import('./types').Logger}


odd... we have import './types.js' above. Why doesn't {Logger} suffice? (I verified that it does not). @michaelfig ?

dckc · 2021-03-05T15:57:22Z

packages/ses-ava/src/ses-ava-test.js

+ * Determine if the argument is a Promise.
+ * (Approximately copied from promiseKit.js)
+ *
+ * @param {any} maybePromise The value to examine


s/any/unknown/ . Don't use any in a type annotation without very good reason.
(just added to https://github.com/Agoric/agoric-sdk/wiki/Code-Review-Checklist)

dckc · 2021-03-05T16:07:05Z

packages/ses-ava/src/types.js

+
+/**
+ * @callback Logger
+ * @param {...any} args


use of any here masked a bug: spead syntax doesn't work here.

dckc · 2021-03-05T17:26:48Z

@kriskowal please (auto) merge this if you approve it.

kriskowal

I’ve submitted my feedback in the form of a commit. I fixed the TypeSciprt imports for types.js by removing import 'ses' from types.js. The evident behavior is that any JS file containing only TS in JSDoc is implicitly imported by all other files in the same directory.

kriskowal · 2021-03-05T18:57:34Z

packages/ses-ava/src/types.js

@@ -0,0 +1,68 @@
+// @ts-check
+import 'ses';


Perhaps try /// <reference types="ses"/> instead. I believe types.js is magical and only does its magic if it does nothing except declare types in JSDoc.

erights requested a review from kriskowal January 1, 2021 06:51

erights self-assigned this Jan 1, 2021

erights mentioned this pull request Jan 4, 2021

WIP Just some debug support experiments off 1905 Agoric/agoric-sdk#2159

Closed

kriskowal requested changes Jan 4, 2021

View reviewed changes

erights marked this pull request as draft January 6, 2021 03:06

erights changed the title ~~Wrap ava test function to log failures to actual console~~ WIP Wrap ava test function to log failures to actual console Jan 6, 2021

erights force-pushed the ava-test-wrapper branch from 70f14e6 to 2885eed Compare January 12, 2021 05:06

kriskowal reviewed Jan 12, 2021

View reviewed changes

packages/ses-ava/rollup.config.js Show resolved Hide resolved

packages/ses-ava/rollup.config.js Outdated Show resolved Hide resolved

packages/ses-ava/rollup.config.js Outdated Show resolved Hide resolved

packages/ses-ava/rollup.config.js Outdated Show resolved Hide resolved

erights force-pushed the ava-test-wrapper branch 2 times, most recently from 62ed696 to bf260a7 Compare January 14, 2021 04:09

dckc reviewed Jan 28, 2021

View reviewed changes

packages/ses-ava/src/types.js Show resolved Hide resolved

erights assigned dckc Jan 29, 2021

dckc mentioned this pull request Feb 17, 2021

error details missing from ava console #577

Closed

erights added the debugging support label Feb 18, 2021

erights mentioned this pull request Feb 22, 2021

Some tests need text comparison of console output. #579

Open

fix: pristine state from scripts/repackage.sh ses-ava

f8524bc

erights force-pushed the ava-test-wrapper branch 2 times, most recently from 9945d8e to edbf3a1 Compare March 5, 2021 00:40

erights requested review from kriskowal and dckc March 5, 2021 04:51

erights changed the title ~~WIP Wrap ava test function to log failures to actual console~~ Wrap ava test function to log failures to actual console Mar 5, 2021

erights marked this pull request as ready for review March 5, 2021 04:52

erights marked this pull request as draft March 5, 2021 04:57

erights force-pushed the ava-test-wrapper branch 3 times, most recently from bcda0b8 to 376711f Compare March 5, 2021 05:17

fix: Implement ava wrapper, and monkey test it.

2cd6ec9

erights force-pushed the ava-test-wrapper branch from 376711f to 2cd6ec9 Compare March 5, 2021 05:31

erights mentioned this pull request Mar 5, 2021

Variation of ses-ava test with deep stacks Agoric/agoric-sdk#2582

Closed

erights and others added 2 commits March 4, 2021 22:23

fix: new TODO comment

842c33f

style(ses-ava): use unknown rather than any in type annotations

964943c

`@callback` doesn't seem to mix with spread syntax, so we use `@typedef` instead for Logger.

dckc approved these changes Mar 5, 2021

View reviewed changes

fix(ses-ava): Patch up TypeScript types.js imports

59bb6b4

kriskowal reviewed Mar 6, 2021

View reviewed changes

kriskowal marked this pull request as ready for review March 6, 2021 04:23

kriskowal approved these changes Mar 6, 2021

View reviewed changes

kriskowal merged commit 576fa9b into master Mar 6, 2021

kriskowal deleted the ava-test-wrapper branch March 6, 2021 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap ava test function to log failures to actual console #555

Wrap ava test function to log failures to actual console #555

erights commented Jan 1, 2021 •

edited by dckc

Loading

erights commented Jan 1, 2021

erights commented Jan 1, 2021

kriskowal commented Jan 4, 2021

kriskowal left a comment

erights commented Jan 4, 2021

erights commented Jan 12, 2021

kriskowal left a comment

erights commented Jan 12, 2021

erights commented Feb 22, 2021

erights commented Mar 5, 2021

erights commented Mar 5, 2021

dckc left a comment

dckc Mar 5, 2021

dckc Mar 5, 2021

dckc Mar 5, 2021

dckc Mar 5, 2021

dckc Mar 5, 2021

dckc Mar 5, 2021

dckc Mar 5, 2021

dckc commented Mar 5, 2021

kriskowal left a comment

kriskowal Mar 5, 2021

Wrap ava test function to log failures to actual console #555

Wrap ava test function to log failures to actual console #555

Conversation

erights commented Jan 1, 2021 • edited by dckc Loading

erights commented Jan 1, 2021

erights commented Jan 1, 2021

kriskowal commented Jan 4, 2021

kriskowal left a comment

Choose a reason for hiding this comment

erights commented Jan 4, 2021

erights commented Jan 12, 2021

kriskowal left a comment

Choose a reason for hiding this comment

erights commented Jan 12, 2021

erights commented Feb 22, 2021

erights commented Mar 5, 2021

erights commented Mar 5, 2021

dckc 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

dckc commented Mar 5, 2021

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights commented Jan 1, 2021 •

edited by dckc

Loading