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

move fake-vat-data test helper to @agoric/vat-data #6834

Open
warner opened this issue Jan 23, 2023 · 5 comments
Open

move fake-vat-data test helper to @agoric/vat-data #6834

warner opened this issue Jan 23, 2023 · 5 comments
Assignees
Labels
enhancement New feature or request hygiene Tidying up around the house liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE

Comments

@warner
Copy link
Member

warner commented Jan 23, 2023

What is the Problem Being Solved?

To reduce a few dependency cycles (and improve lerna/CI q.v. #6823), we'd like to rearrange the way our test helpers are hosted.

In agoric-sdk, we roughly have three (or four) categories of unit tests:

  • (0): tests that only need AVA, these are pretty rare because we use harden and assert in almost everything
  • 1: tests that need just SES and AVA
    • this includes any test which imports any code that imports @agoric/assert, or uses the global assert or harden, since these get added to the global environment as part of @endo/init
    • if AVA didn't modify the environment so much, tests in this category could just do import '@endo/init'; import test from 'ava';
    • but it does, so we need a special "trusted shim" to load AVA in the right way
  • 2: tests that need SES/AVA but also VatData
    • this includes any test which imports any code that imports @agoric/vat-data, which uses a VatData global
    • this provides defineKind, and the various virtual collections
  • 3: tests that need to run a full swingset kernel
    • these tests import from @agoric/swingset-vat, call buildVatController(), and define some vats

So far, most tests in the first and second categories do:

import { test } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';

which provides all of SES, AVA, and VatData. However A: it's overkill, and B: it causes cyclic dependencies, which apparently interfere with some cleverness that lerna can apply when the dependency graph is a DAG.

Description of the Design

First, we're thinking of moving the VatData (fakeVirtualStuff) helpers out of packages/SwingSet/ and into packages/vat-data. So all tests in the second category could do something like `import test from '@agoric/vat-data/tools/prepare-vatdata-ava.js``.

This would be facilitated by moving liveslots out of packages/SwingSet/ and into its own package, tenatively named packages/swingset-liveslots/. I've been working on this as part of #6596, since each worker type will have its own package, and they'll all need to import liveslots. With this complete, packages/vat-data does not depend upon swingset, it only depends upon liveslots. Swingset depends upon liveslots, until 6596 is done and swingset depends on the worker packages instead.

Then, we'd like to make a new tool that satisfies the first category of tests. This might want to live in the https://github.com/endojs/ monorepo, in a package speculatively named @endo/ava. This would move it out of swingset, thus allowing tests that need SES+AVA but not VatData or swingset to avoid any dependencies on either.

Test Plan

Unit tests would need updating, but then the fact that they continue to work will be test enough.

@warner warner added enhancement New feature or request SwingSet package: SwingSet hygiene Tidying up around the house labels Jan 23, 2023
@turadg
Copy link
Member

turadg commented Jan 23, 2023

Relevant to case 1: endojs/endo#1235

@erights
Copy link
Member

erights commented Jan 24, 2023

I was curious what the current dependence of `SwingSet/tools/prepare-test-env-ava.js' is on any of the rest of SwingSet. All I found is:

// Does not import from a module because we're testing the global env
/* global globalThis */
export const VatData = globalThis.VatData;
assert(VatData);

Is there any other dependency? Is there some significance to these four lines that would prevent us from just deleting them?

@erights
Copy link
Member

erights commented Jan 24, 2023

That said, I don't have any objections to moving the fake virtual support into vat-data. In fact, this sounds like a good idea regardless.

@warner
Copy link
Member Author

warner commented Jan 24, 2023

I only see two other places (zoe/test/unitTests/test-{makeKind,zoe-env}.js) which take advantage of this VatData export, and those should be able to get what they need from @agoric/vat-data. So those four lines aren't likely to be important.

What matters is the rest of the side-effects created by the other imports inside that file, and the export const test = wrapTest(rawTest) that it provides in lieu of the normal import test from 'ava'.

Those tests (category two, from above) need SES to be configured, AVA to work (which requires the vetted shim), and a fake VatData to be added to the global (which does not require a vetted shim, it just needs to be added before any other code tries to read it). To make that combination usable by those tests, all three things want to happen as a side-effect of a single import. So prepare-test-env-ava.js was born. I think it originally started out in swingset/ because most of the VatData support lives there (inside src/liveslots/, in particular), but it doesn't need to stay there.

@warner warner self-assigned this Jan 24, 2023
@warner warner added the liveslots requires vat-upgrade to deploy changes label Jan 24, 2023
@warner
Copy link
Member Author

warner commented Feb 1, 2023

@gibson042 I think this overlaps a lot with the #6703 work that we just discussed, so assigning it to you

@ivanlei ivanlei added the vaults_triage DO NOT USE label Feb 13, 2023
@warner warner removed their assignment Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hygiene Tidying up around the house liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE
Projects
None yet
Development

No branches or pull requests

5 participants