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

migrate testing to AVA #1568

Closed
35 tasks done
warner opened this issue Aug 20, 2020 · 8 comments
Closed
35 tasks done

migrate testing to AVA #1568

warner opened this issue Aug 20, 2020 · 8 comments
Assignees
Labels
test tooling repo-wide infrastructure

Comments

@warner
Copy link
Member

warner commented Aug 20, 2020

Our current best-looking result from the #1335 survey is to use AVA instead of tap/tape.

This ticket is about performing that migration.

Questions for me to answer first:

  • does AVA play nicely with ESM (without -r esm)
    • we'll be adding an AVA config to add -r esm for all packages that are not yet set to type: module
  • what will be our new best-practices for adding console.log to debug test problems
  • write up instructions to run just one test
  • write up instructions to run under the debugger

I'm hoping to find a way to convert one modules at a time, for smaller PRs, but I won't let that stop me.

I'll be drawing from @dtribble 's #1447 PR.

  • assert
  • install-ses
  • import-manager
  • sparse-ints
  • registrar
  • store
  • weak-store
  • acorn-eventual-send
  • bundle-source
  • import-bundle
  • eventual-send
  • promise-kit
  • transform-eventual-send
  • tame-metering
  • transform-metering
  • install-metering-and-ses
  • marshal
  • same-structure
  • captp
  • stat-logger
  • swing-store-lmdb
  • swing-store-simple
  • SwingSet
  • swingset-runner
  • ERTP
  • spawner
  • sharing-service
  • zoe
  • dapp-svelte-wallet/ui
  • dapp-svelte-wallet/api
  • dapp-svelte-wallet
  • cosmic-swingset
  • agoric-cli
  • deployment
  • notifier
@warner warner added the tooling repo-wide infrastructure label Aug 20, 2020
@warner warner self-assigned this Aug 20, 2020
@warner
Copy link
Member Author

warner commented Aug 20, 2020

@michaelfig points out that we have non-test files with names like testHelpers.js that should not be treated as tests. So our files config should be packages/*/test/**/test-*.js, not packages/*/test/**/test*.js.

I'll look to see if we also have test.js anywhere (which would be appropriate for a small package that only has one test file). We'll either need to rename those to something longer, or augment files to a second glob of packages/*/test/**/test.js.

Either way, we need to update our coding-standards document to recommend the naming scheme.

@warner
Copy link
Member Author

warner commented Aug 20, 2020

From what I can tell in the config docs, AVA processes configuration data from exactly one of three places:

  • the ava section of the nearest package.json
  • an ESM-format ava.config.js file next to the nearest package.json (this is always treated as ESM even if the package.json doesn't saytype: "module", and cannot do imports, nor does it need -r esm to read, so I'm not convinced it's being processed by Node's normal import path)
  • a CJS-format ava.config.cjs file next to the nearest package.json

The .js-format files are not allowed to import anything. And AVA doesn't merge configs like eslint does (it doesn't even look outside the current package). Which means some duplication in our configs.

We want to enable both a top-level do-all-packages-in-parallel yarn test target, and individual one-package-at-a-time yarn test targets. The former needs to be defined in the top-level package.json, and will need an ava invocation that targets packages/*/test/**/test-*.js, using the same options for everything. The latters will be defined in each package/$PACKAGENAME/package.json, with targets like test/**/test-*.js, and can have separate options for each package. All package.json files, top-level and per-package, will have a scripts: section with a simple test: "ava" entry.

We could factor out the per-package configs into a separate shared file (perhaps agoric-sdk/ava-shared-config.js) and use test: "ava --config ../../ava-shared-config.js". Running yarn ava in a package would fail (it would use the default config), but yarn test would work as expected.

When we finally move to ESM (#527), there will be a conflict between heterogenous packages (some ESM, some CJS+-r esm) and a top-level ava invocation, because that top-level ava command will either use requires: ["esm"] (aka -r esm) for everything, or for nothing. If we want to space out the #527 landing somewhat, updating one package at a time, we may have to downgrade our top-level yarn test invocation from running ava (run all packages in parallel) to running yarn workspaces test (run one package at a time), until we complete the ESM transition.

@warner
Copy link
Member Author

warner commented Aug 20, 2020

I'm going to proceed one package at a time, leaving the top-level yarn test mapped to yarn workspaces test, until all packages are converted. Then we can make a final change to the top-level package.json and enable full parallelism.

@warner
Copy link
Member Author

warner commented Aug 20, 2020

I've checked a simple package (with no dependencies) with "type": "module" under AVA (removing the require: ["esm"] from the AVA config) and it seems to work. I also tried the same with the require: ["esm"] and it still works, which suggests that maybe -r esm is more tolerant of actually-declared-ESM files than I thought, and offers some hope for the heterogenous-packages conflict described above.

@warner
Copy link
Member Author

warner commented Aug 20, 2020

AVA rejects ava --config ../../ava-shared-config.js: it's really insistent upon having the config file live next to the nearest package.json. That rules out a shared config, without resorting to symlinks or pre-test steps that create that local file (by copying it from a shared source). It's probably not a big deal, there aren't a lot of options to change anyways, and you can always make per-invocation changes by just adding them to the yarn script command line (e.g. yarn test --verbose or yarn test -m '*testname to run*').

This was referenced Aug 22, 2020
warner added a commit that referenced this issue Aug 23, 2020
chore: update 'spawner' package to use AVA

refs #1568
warner added a commit that referenced this issue Aug 23, 2020
warner added a commit that referenced this issue Aug 23, 2020
chore: update 'dapp-svelte-wallet' package to use AVA

refs #1568
warner added a commit that referenced this issue Aug 23, 2020
warner added a commit that referenced this issue Aug 24, 2020
warner added a commit that referenced this issue Aug 24, 2020
fix: actually test cosmic-swingset files in parallel

refs #1568
warner added a commit that referenced this issue Aug 24, 2020
chore: update 'sharing-service' package to use AVA

refs #1568
warner added a commit that referenced this issue Aug 24, 2020
warner added a commit that referenced this issue Aug 25, 2020
update swingset-runner/swingstore to AVA

refs #1568
@warner
Copy link
Member Author

warner commented Sep 1, 2020

All packages have been converted. The remaining step is make the top-level yarn test target use AVA in parallel across all packages, rather than doing a yarn workspaces test which runs one package at a time.

This might improve reporting: AVA would be in control of all tests. Success/failure/progress would be reported one test file at a time. AVA would not even be aware of what package contains each test file. And it could improve performance slightly, by increasing the opportunity for parallelism.

The complication is that we can't use the same AVA config for all our tests, specifically the ava.require=[] list. We need -r esm for everything (for now). Of our 109 test-*.js files, we need @agoric/install-ses for 83 of them, @agoric/install-metering-and-ses for 10, and neither for 16. @dtribble added ava.require=['esm', '@agoric/install-ses'] for the 6 in ERTP with good results, and we could do that for most of the rest (adding just SES isn't likely to cause any problems).

However the 10 that actually exercise metering couldn't be included under that command. We'd have to have two separate ava invocations, one with the default (using the package.json config of ava.require=['esm', '@agoric/install-ses']), and a second with an alternate config file that used ava.require=['esm', '@agoric/install-metering-and-ses']. These would not run in parallel: we'd do all the non-metering tests in one pass, then all the metering tests in a second pass. We'd move all the metering-based tests into separate directories, probably PACKAGENAME/test/* and PACKAGENAME/metering-test/*, so the top-level config file could find the right ones.

We'd need to remove all the manual import '@agoric/install-ses' / import '@agoric/install-metering-and-ses' lines from our test files, which means they can only be run under AVA (which is the case anyways, because they import test from 'ava', which balks otherwise). but it would also change our process for running a single test file: instead of just yarn test test/test-foo.js, if the test is a metering test, you'd need to run something like yarn test-metering metering-test/test-bar.js. Running yarn test metering-test/test-bar.js would result in an error becauase the metering code would not get installed.

@dtribble dtribble added the test label Jan 25, 2022
@warner
Copy link
Member Author

warner commented Feb 7, 2022

We no longer use install-metering-and-ses, so I think that blocker is gone.

@warner
Copy link
Member Author

warner commented Feb 7, 2022

closing this in favor of the smaller #4493

@warner warner closed this as completed Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test tooling repo-wide infrastructure
Projects
None yet
Development

No branches or pull requests

2 participants