-
Notifications
You must be signed in to change notification settings - Fork 81
feat(ses-ava): Multiplex configurations #2987
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
Conversation
756468c to
eb857e3
Compare
packages/ses-ava/README.md
Outdated
| ``` | ||
|
|
||
| This relies on `@endo/ses-ava/prepare-endo-config.js` to initialize an | ||
| Endo envrionment, including the SES shims and Eventual Send shim, and also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Endo envrionment, including the SES shims and Eventual Send shim, and also | |
| Endo environment, including the SES shims and Eventual Send shim, and also |
packages/ses-ava/test.js
Outdated
| export const register = newTest => { | ||
| test = newTest; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think register is a bit too generic.
| export const register = newTest => { | |
| test = newTest; | |
| }; | |
| /** | |
| * @template [Context=unknown] | |
| * @param {import('ava').TestFn<Context>} newTestFn | |
| */ | |
| export const replaceAvaTestFn = newTestFn => { | |
| test = newTestFn; | |
| }; |
packages/ses-ava/README.md
Outdated
| The `ses-ava` command consumes the `"ava"` and (new) `"avaConfigs"` properties | ||
| in `package.json` to discover and name the supported configurations and infer | ||
| flags like `--only-configname` and `--no-configname` for each, where the `"ava"` | ||
| configuration is the `default`, if present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rename "avaConfigs" to something that makes clear that the functionality is not being provided by ava itself.
| The `ses-ava` command consumes the `"ava"` and (new) `"avaConfigs"` properties | |
| in `package.json` to discover and name the supported configurations and infer | |
| flags like `--only-configname` and `--no-configname` for each, where the `"ava"` | |
| configuration is the `default`, if present. | |
| The `ses-ava` command consumes the `"ava"` and (new) `"sesAvaConfigs"` properties | |
| in `package.json` to discover and name the supported configurations and infer | |
| flags like `--only-configname` and `--no-configname` for each, where the `"ava"` | |
| configuration is the `default`, if present. |
Also note that such a wrapper is probably useful for more than just ses-ava... should we use "multiAvaConfigs" in anticipation of possible generalization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered sesAvaConfigs to avoid name-squatting in Ava’s domain, so I think that’s two votes. We can update our packages if ava upstream’s the feature.
packages/ses-ava/README.md
Outdated
| With this configuration, `ses-ava ...args --no-lockdown` and `ses-ava ...args | ||
| --only-unsafe` would both just run the `unsafe` configuration. | ||
| Using `ses-ava` under `c8` allows all configurations to cover used code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What controls which arguments are interpreted by ses-ava vs. passed along? Argument hockey against a moving target gets dicey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m open to other options. My intuition is consistent with yours until I considered the ergonomics. I’m convinced, given the absence of better alternatives, that we should just use a more elaborate pair of prefixes to avoid collisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with fully virtualizing the ava flags. The new code intercepts the common flags and passes them down. -- allows for pass-thru of arbitrary additional flags, but isn’t necessary in any of the common cases. I went with --no-config- and --only-config- since they don’t collide, are unlikely to collide, and we can compensate for collisions later without breaking anything.
eb857e3 to
226bd4f
Compare
| const noKey = noFlags.get(arg); | ||
| const onlyKey = onlyFlags.get(arg); | ||
| if (noKey) { | ||
| no.add(noKey); | ||
| } else if (onlyKey) { | ||
| only.add(onlyKey); | ||
| } else { | ||
| args.push(arg); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this dynamic sensitivity can make for awful debugging, and hinders future evolution. Suggestion: just claim everything that starts with - up to the first --.
// Claim all options up to the first `--` (if any), but pass through every
// positional argument.
const args = [];
const argsIterator = process.argv.slice(2)[Symbol.iterator]();
for (const arg of argsIterator) {
if (arg === '--') {
args.push(...argsIterator);
break;
} else if (!arg.startsWith('-')) {
args.push(arg);
continue;
}
if (arg.startsWith('--no-')) {
no.add(arg.slice('--no-'.length));
} else if (arg.startsWith('--only-')) {
only.add(arg.slice('--only-'.length));
} else {
throw Error(`Unsupported argument: ${arg}`);
}
}
for (const key of no) {
if (Object.hasOwn(avaConfigs, key)) continue;
throw Error(`Cannot find excluded configuration: ${key}`);
}
for (const key of only) {
if (Object.hasOwn(avaConfigs, key)) continue;
throw Error(`Cannot find exclusive configuration: ${key}`);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Downside: You then need to yarn test -- -m case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also downside: You cannot yarn test -m 'case name' --only-lockdown and iterate on the final argument.
packages/ses-ava/src/command.js
Outdated
| args.push(arg); | ||
| } | ||
| } | ||
| const configs = (only.size > 0 ? only : all).difference(no); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--only-foo with --no-foo should probably be rejected, but if not then letting --no-foo win seems odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote with safeties.
546fbed to
84e6784
Compare
87d00cd to
342ac49
Compare
packages/ses-ava/README.md
Outdated
| In the root of the Endo repository, look the the `ava-*.config.mjs` modules | ||
| for example configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| In the root of the Endo repository, look the the `ava-*.config.mjs` modules | |
| for example configurations. | |
| In the root of the Endo repository, look the at the `ava-*.config.mjs` modules | |
| for example configurations. |
packages/ses-ava/README.md
Outdated
| a regular dependency, so it you include @endo/ses-ava as a regular | ||
| dependency, bundlers might bundle your code with all of Ava. | ||
|
|
||
| SES-Ava rhymes with Nineveh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit (throughout the documentation): https://github.com/avajs/ava/tree/main#how-is-the-name-written-and-pronounced
| SES-Ava rhymes with Nineveh. | |
| SES-AVA rhymes with Nineveh. |
| * The ses-ava command will by default run all tests in every mode but allows | ||
| * the user to pass --only-* and --no-* at any argument position for any of | ||
| * the named configurations to filter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: rather than dynamically defining flags from configuration, use repeatable argument-bearing options.
| * The ses-ava command will by default run all tests in every mode but allows | |
| * the user to pass --only-* and --no-* at any argument position for any of | |
| * the named configurations to filter. | |
| * The ses-ava command will by default run all tests in every mode but allows | |
| * the user to pass --only <name> and --exclude <name> (or shorthands -o <name> | |
| * and -x <name>) at any argument position for any of the named configurations | |
| * to filter. |
(and if accepted, remember to also update the test descriptions and documentation in e.g. README.md)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure I will regret this, but in the interest of using time efficiently, I’m going to merge without addressing this one, though it is absolutely a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Captured as #2998.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, I just went ahead and submitted a fix: #2999
packages/ses-ava/src/command.js
Outdated
| const singlePassThrough = new Set([ | ||
| '-t', | ||
| '--tap', | ||
| '-s', | ||
| '--serial', | ||
| '-u', | ||
| '--update-snapshots', | ||
| '--no-worker-threads', | ||
| '--verbose', | ||
| '--no-verbose', | ||
| ]); | ||
|
|
||
| const doublePassThrough = new Set(['-m', '--match', '--node-arguments']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Composite suggestions (format short/long aliases on the same line; include options for ava debug; rename these two variables):
| const singlePassThrough = new Set([ | |
| '-t', | |
| '--tap', | |
| '-s', | |
| '--serial', | |
| '-u', | |
| '--update-snapshots', | |
| '--no-worker-threads', | |
| '--verbose', | |
| '--no-verbose', | |
| ]); | |
| const doublePassThrough = new Set(['-m', '--match', '--node-arguments']); | |
| // We pass these flags directly to ava. | |
| const passThroughFlags = new Set([ | |
| ...['-s', '--serial'], | |
| ...['-t', '--tap'], | |
| ...['-T', '--timeout'], | |
| ...['-u', '--update-snapshots'], | |
| ...['-v', '--verbose'], | |
| '--no-worker-threads', | |
| '--no-verbose', | |
| // Specific to ava debug. | |
| '--break', | |
| ]); | |
| // We read a single argument for each of these options and pass the pair | |
| // directly to ava. | |
| const passThroughArgOptions = new Set([ | |
| ...['-m', '--match'], | |
| '--node-arguments', | |
| // Specific to ava debug. | |
| '--host', | |
| '--port', | |
| ]); |
packages/ses-ava/src/command.js
Outdated
| const onlyKey = onlyFlags.get(arg); | ||
| if (singlePassThrough.has(arg)) { | ||
| passThroughArgs.push(arg); | ||
| } else if (doublePassThrough.has(arg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else if (doublePassThrough.has(arg)) { | |
| } else if (doublePassThrough.has(arg)) { | |
| // Note that we don't support GNU-style `--name=value` syntax. |
packages/ses-ava/src/command.js
Outdated
| foundNextArg: { | ||
| for (const nextArg of argsIterator) { | ||
| passThroughArgs.push(arg, nextArg); | ||
| break foundNextArg; | ||
| } | ||
| throw new Error(`Expected argument after ${arg}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like overkill in comparison to just calling next().
| foundNextArg: { | |
| for (const nextArg of argsIterator) { | |
| passThroughArgs.push(arg, nextArg); | |
| break foundNextArg; | |
| } | |
| throw new Error(`Expected argument after ${arg}`); | |
| } | |
| const { done, value } = argsIterator.next(); | |
| if (done) throw new Error(`Expected argument after ${arg}`); | |
| passThroughArgs.push(arg, value); |
packages/ses-ava/src/command.js
Outdated
| if (failFast) { | ||
| process.exit(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (failFast) { | |
| process.exit(); | |
| } | |
| if (failFast && process.exitCode) { | |
| process.exit(); | |
| } |
packages/ses-ava/src/command.js
Outdated
|
|
||
| // Debug will only apply to the first matching configuration. | ||
| if (debug) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be misleading, and also frustrating in cases where I cared about debugging against multiple configurations (or just not the first one). The child processes are run serially, so we might as well just apply debug to all of them.
| // Debug will only apply to the first matching configuration. | |
| if (debug) { | |
| break; | |
| } |
| "sesAvaConfigs": { | ||
| "one": "test/_ava1.config.js", | ||
| "two": "test/_ava2.config.js", | ||
| "raw": "test/_ses-ava-is-ava.config.js" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These config names seem unnecessarily generic; can we make the naming (and thus comprehension of how this file expects testing to proceed) more clear? A possibility:
| "sesAvaConfigs": { | |
| "one": "test/_ava1.config.js", | |
| "two": "test/_ava2.config.js", | |
| "raw": "test/_ses-ava-is-ava.config.js" | |
| }, | |
| "sesAvaConfigs": { | |
| "custom-env": "test/_custom-env.ava-config.js", | |
| "must-ignore": "test/_must-ignore.ava-config.js", | |
| "raw": "test/_raw.ava-config.js" | |
| }, |
| } | ||
|
|
||
| // Execute configurations serially. | ||
| for (const config of configs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: tack on our own detail.
| for (const config of configs) { | |
| for (const config of configs) { | |
| console.warn(`[ses-ava] config:`, config); |
(or similar)
ecec978 to
df3ba90
Compare
df3ba90 to
2b0119f
Compare
Refs: #2983
Preface
There are a number of ways an Endo environment can be configured. We consider it table stakes that being able to move between these configurations should have as little friction as possible. Code written that works in one configuration should work in others.
The important configuration is the strict, default post-lockdown environment. This is where most of our test coverage occurs today. However, we also support a lockdown mode
hardenTaming: unsafe, through@endo/init/unsafe-fast.js, which lets single-tenant vats like the Agoric chain supervisor run fast and rely on pinky-swear immutability of hardened data and interfaces. We wish to have test coverage for this mode, and to that end, we wish to run our existing test suites in multiple modes.Description
In order to better cover multiple environment configurations, this change introduces a
ses-avacommand and machinery to let us reuse tests regardless of whetherlockdownis called. Then, adopts this feature in every applicable Endo package, in every configuration for which tests currently pass.The new
ses-avacommand passes most flags through toava, and runsavaonce for each of the configurations inpackage.json, including the defaultavaif present, then other configuration files named in asesAvaConfigssection. Theses-avacommand then goes on to add--only-config-${x}and--no-config-${x}to include or exclude configurations in a test run, to help facilitate isolation of test failures.In this change, we update every applicable workspace’s
package.jsonand provide a bank of common AVA configuration files at the root of the repository for each workspace to use. Without modification, we adopt all the configurations that are currently working, with the expectation that we will support 2 or 3 such configurations for most packages going forward.Security Considerations
None.
Scaling Considerations
Tests will run longer.
Documentation Considerations
See ses-ava/README and ses-ava/NEWS.
Testing Considerations
In entire.
Compatibility Considerations
This will increase test coverage for supported scenarios.
Upgrade Considerations
None.