NOT FOR MERGE: hack on zoe test #1629
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
avajs/ava#2262 seems related |
We were seeing random test failures in the zoe unit tests (specifically
test-offerSafety.js) that looked like:
```
Uncaught exception in test/unitTests/test-offerSafety.js
/home/runner/work/agoric-sdk/agoric-sdk/packages/weak-store/src/weakStore.js:5
import { assert, details, q } from '@agoric/assert';
^^^^^^
SyntaxError: Cannot use import statement outside a module
✖ test/unitTests/test-offerSafety.js exited with a non-zero exit code: 1
```
or:
```
Uncaught exception in test/unitTests/test-offerSafety.js
/home/runner/work/agoric-sdk/agoric-sdk/packages/zoe/test/unitTests/setupBasicMints.js:1
import { makeIssuerKit } from '@agoric/ertp';
SyntaxError: Cannot use import statement outside a module
✖ test/unitTests/test-offerSafety.js exited with a non-zero exit code: 1
```
under various versions of Node.js. The error suggests that `-r esm` was not
active (the error site tried to import an ESM-style module, and failed
because the import site was CJS not ESM), however error site itself was in a
module, meaning `-r esm` was active up until that moment. This makes no
sense.
The AVA runner has had some amount of built-in support for ESM, in which it
uses Babel to rewrite test files (but perhaps not the code being tested). If
that were in effect, it might explain how `test-offerSafety.js` could be
loaded, but `weakStore.js` could not. It is less likely to explain how
`setupBasicMints.js` could be loaded but `makeIssuerKit` could not, unless
AVA somehow believes that `setupBasicMints.js` is a different category of
file than the ones pulled from other packages.
In any case, I believe we're bypassing that built-in/Babel support, by using
their recommended `-r esm` integration recipe (package.json has
`ava.require=['esm']`). However the AVA "ESM Plan"
(avajs/ava#2293) is worth reading, if only
for our future move-to-native-ESM plans (#527).
So my hunch here is that the `-r esm` module's cache is not safe against
concurrent access, and AVA's parallel test invocation means there are
multiple processes reading and writing to that cache in parallel. Zoe has
more source files than most other packages, which might increase the
opportunity for a cache-corruption bug to show up. This sort of bug might not
show up locally because the files are already in the cache, whereas CI may
not already have them populated.
This patch adds `ESM_DISABLE_CACHE=true` to the environment variables used in
all tests, in an attempt to avoid this hypothetical bug. Stale entries in
this cache has caused us problems before, so most of us have the same setting
in our local shells.
Another potential workaround would be to add `--serial` to the `ava`
invocation, however the Zoe test suite is large enough that we really to want
the parallelism, just to make the tests finish faster.
This patch also increases the Zoe test timeout to 10m, just in case. I
observed a few tests taking 1m30s or 1m40s to complete, and the previous
timeout was 2m, which was too close to the edge.
warner
force-pushed
the
1568-warner-test
branch
from
4eda7f0 to
ec8c456
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOT FOR MERGE
For some reason, in CI (but not locally), test-offerSafety.js seems to forget that we've added
-r esmhalfway through the test. Like, it loads one ESM module just fine, but then that module tries to doimportand suddenly Node starts complaining aboutSyntaxError: Cannot use import statement outside a module. From within a module!. I've seen this happen in different modules, under different versions of Node (12.something and 14.8.0).