Impedence mismatch between swingset-runner and Zoe tests

[FUDCo]

A note on adapting Zoe tests in agoric-sdk/zoe/tests/swingsetTests/zoe to run under swingset-runner:

Broadly speaking, there were two kinds of issues that needed to be addressed to be able to do this:

1: Different import contexts

The various Zoe test vats' source files assume they are located in particular locations inside the zoe project tree and do imports using relative paths based on that assumption. If we could load those files directly from their native location, this would not be an issue, but unfortunately they have to be tweaked slightly to work in swingset-runner.

2: No test driver

We're not running inside the unit test framework, and thus we don't have it to kick around any more (a) set things up, (b) parameterize what is to happen, and (c) provide an execution context. Each of those unpacks into a separate problem. These problems are all minor, but the changes to address them are the reason we can't just use the vat files directly.

What was needed

Since the changes required were modest, it would be nice to consider if the two versions of the vat definitions could be unified to use a single common code base. Until then, the necessary adaptions were as follows:

I copied the vat source files (bootstrap.js and the various vat-*.js files) into a new directory, agoric-sdk/swingset-runner/demo/zoeTests and then modified as follows.

Problem 1

Dealing with this varied a little depending on the import dependency in question. I decided it was OK to use import paths that reached into the Zoe src directory, on the theory that these files were part of the published package, but not files from elsewhere in the Zoe tree (such as the test directories).

agoric-sdk/zoe/tools/manualTimer.js and agoric-sdk/zoe/tests/swingsetTests/helpers.js could be copied into the swingset-runner zoeTests directory and used without modification. The vats then had to be changed to import ./manualTimer and ./helpers instead of ../../../tools/manualTimer and ../helpers respectively.

The client support library was pulled in by importing @agoric/zoe/src/clientSupport instead of ../../../src/clientSupport and the Zoe library from @agoric/zoe instead of ../../../src/zoe.

Problem 2a

The issue here was the need to generate the bundles for the contracts. The test-zoe.js script did this prior to running the tests, but the only code that swingset-runner will be running will be inside vats. In principle the bundling could be done by the bootstrap vat, but a simpler approach was to extract the relevant code fragment from test-zoe.js into a standalone executable I called prepareContracts.js, which needs to be executed once prior to trying to run things in swingset-runner (and rerun any time the contract code changes, which introduces another manifestation of the yarn build problem, but since this is POC code I chose the simpler expedient).

Problem 2b

The problem here is that the extent argument that gets passed to the bootstrap vat is generated by the test script prior to the vat existing. Instead of being given to the bootstrap vat directly it is handed to buildVatController as a parameter to swingset creation, which means we must know which test is being run before we create the bootstrap vat. But swingset-runner can't do that since it has no varies-per-swingset configuration. Also, the argument is an array of arrays of numbers, whereas swingset-runner is passing command line arguments which are always strings. The solution was to pass the extent as a command line parameter string, which required changing the relevant line of the bootstrap vat from

    const [testName, startingExtents] = argv;

to

    const [testName, startingExtentsStr] = argv;
    const startingExtents = JSON.parse(startingExtentsStr);

And then (and this is the really ugly part) providing the extents array as a stringified command line parameter when you want to execute a test.

A long term solution would be to get this data from a file that, say, both the Zoe tests and the swingset-runner version could import, but I don't know that there is a long term that needs to be satisfied here so just living with the ugliness may be more pragmatic.

Problem 2c

The test harness extracts the run log from the kernel state after test execution and compares it to a reference sample, whereas what we want to do is print it to the console as the test is running. This requires providing an alternative log function to wrap helpers.log with a version that also echoes to the console. This in turn means that every vat setup function that has:

    E => build(E, helpers.log),

must instead have

   E => build(E, makePrintLog(helpers.log)),

where makePrintLog is the wrapper function of my own that then needs to also get imported into each of the vats. As Brian points out, this opens a whole can of worms about what logging is for and how it should work and what we really want in that arena; the questions raised there are important but not on the critical path for this particular bit of work.

[FUDCo]

@warner @katelynsills @Chris-Hibbert -- A note on the contortions I went through to run the Zoe test code outside the test framework. Nothing actionable for you here, but if you have thoughts or reactions I wouldn't mind hearing them.

[michaelfig]

@FUDCo thanks so much for writing this up. It was a good guide for upgrading to newer versions of zoeTests.

[Chris-Hibbert]

Thanks for the write-up.

In general, I'm supportive of changes to the source code that makes them more testable. If we can move in the direction of Dependency Injection, that makes the code be better factored in general, and is also a huge win for testing.

I think it would be good if we generally wrote our imports to rely more on package paths rather than relative location of source code. This makes more code more independent of refactorings in distant places. Most of the time references from the test code to source files would be improved by making this kind of change.

Driving the tests from config files rather than passing parameter through a circuitous vat-creation path would have a similar effect.

Changing the way logs are created so the tests are written with the same invocation and can be re-used for performance tests or other things is all to the good. It would be fine to change the test code once so people who want to re-use it don't have to make distinct changes multiple times.