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 benchmarkerator and some test support code out of the boot package #8421

Merged
merged 3 commits into from
Nov 18, 2023

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Oct 2, 2023

This moves files around, creating two new packages: benchmark, containing the Benchmarkerator benchmarking framework, and test-support, containing support code that is used by both tests (mostly in boot) and Benchmarkerator. Code which uses the code that was moved has had the relevant import statements updated accordingly.

This completes work that was suggested by reviews to #8312 but which we chose to defer to its own separate PR for reasons of clarity and smoothness of transition to the new code state.

All of this is in support of #8327

@FUDCo FUDCo added performance Performance related issues hygiene Tidying up around the house test labels Oct 2, 2023
@FUDCo FUDCo requested review from turadg and warner October 2, 2023 19:01
@FUDCo FUDCo self-assigned this Oct 2, 2023
@FUDCo FUDCo force-pushed the refactor-bootstrap-test-support branch 4 times, most recently from 73ed138 to 57a8ba1 Compare October 2, 2023 20:36
turadg
turadg previously requested changes Oct 3, 2023
{
"name": "@agoric/test-support",
"version": "0.1.0",
"description": "Support libraries for testing on chain",
Copy link
Member

@turadg turadg Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name test-support sounds much broader than that.

these also don't test on a chain, right? Just a swingset.

I request that you leave these in boot and export them under its tools. @agoric/benchmark can import them from there. I believe that's what you proposed and I supported in #8312 (comment)

If that complicates some other plans, let's chat

Copy link
Contributor Author

@FUDCo FUDCo Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this would complicate other plans. There's little in those files that specifically pertains to the bootstrap tests, but they do contain the current best versions of some bits that are widely replicated throughout our suite of tests in a copy-paste-edit reuse with minor changes pattern. It's our hope to start cleaning that stuff up using common implementations of commonly used test boilerplate, which will want to draw on a common package for that stuff.

Copy link
Contributor Author

@FUDCo FUDCo Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turadg and I had a long chat about this. He feels quite strongly that the stuff about setting up a mock chain really belongs in the boot package since that's really what that package is all about. I'm not 100% sure I agree, but on the other hand I don't really have a very good counterargument either. We did agree that if the stuff is being used from the outside the package it does not belong buried layers down in the test directory. Instead, we're putting the support infra into the tools directory. The pieces of support code that I had flagged as being much more widely useful beyond just the bootstrap tests pertain to convenience sugaring for tests invoking the controller. This stuff has been broken out into a separate run-utils module and placed into SwingSet/tools, where it will be available to tests that are using SwingSet but not entraining the rest of the chain machinery. The test-support package is now gone. I will revise the PR title and descriptive commentary to match this resolution.

I think we still have some confusion to sort out with respect to the somewhat conflicted meaning of the tools directory, as we have used it to hold both "ancillary executables that are useful in a standalone mode" as well as "utility code that is outside the supported package APIs", sometimes even within the scope of a single package. However, this PR is not the place to resolve that.

@FUDCo FUDCo changed the title Move benchmarkerator and test support code into their own packages Move benchmarkerator and some test support code outside the boot package Oct 5, 2023
@FUDCo FUDCo changed the title Move benchmarkerator and some test support code outside the boot package Move benchmarkerator and some test support code out of the boot package Oct 5, 2023
@FUDCo FUDCo force-pushed the refactor-bootstrap-test-support branch 5 times, most recently from 46093ff to e19d95f Compare October 6, 2023 23:13
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting aside benchmarkerator interface questions for now, I still have some suggestions.

packages/SwingSet/tools/run-utils.ts Outdated Show resolved Hide resolved
packages/SwingSet/tools/run-utils.ts Outdated Show resolved Hide resolved
packages/SwingSet/tools/run-utils.ts Outdated Show resolved Hide resolved
packages/SwingSet/tools/run-utils.ts Outdated Show resolved Hide resolved
packages/SwingSet/tools/run-utils.ts Show resolved Hide resolved
Comment on lines +140 to +153
EV.get = presence =>
new Proxy(harden({}), {
get: (_t, pathElement, _rx) =>
queueAndRun(() =>
controller.queueToVatRoot('bootstrap', 'awaitVatObject', [
presence,
[pathElement],
]),
),
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumption of a bootstrap-vat awaitVatObject method is giving off some bad vibes; is there a way to do it with just the controller? Untested idea:

Suggested change
EV.get = presence =>
new Proxy(harden({}), {
get: (_t, pathElement, _rx) =>
queueAndRun(() =>
controller.queueToVatRoot('bootstrap', 'awaitVatObject', [
presence,
[pathElement],
]),
),
});
/**
* Derives a value suitable for immediate interaction from a vat-originating value:
* the fulfillment value of a fulfilled promise, or
* the rejection value of a rejected promise, or
* a non-promise value itself.
* If `value` is an unsettled promise, a synchronous error will be thrown.
*
* @param {any} presence
* @returns {{status: 'fulfilled' | 'rejected' | undefined, value: any}}
*/
const presenceSettlement = (presence, errPrefix = '') => {
if (passStyleOf(presence) === 'promise') {
const kpid = krefOf(presence);
const status = controller.kpStatus(kpid);
switch (status) {
case 'fulfilled':
// falls through
case 'rejected':
return { status, value: kunser(controller.kpResolution(kpid)) };
case 'unresolved':
throw Fail(`${b(errPrefix)}unsettled value for ${q(kpid)}`;
default:
throw Fail`${b(errPrefix)}unknown promise status ${q(kpid)} ${q(status)}`;
}
}
return { status: undefined, value: presence };
};
EV.get = presence =>
new Proxy(harden({}), {
get: async (_t, key, _rx) => {
await queueAndRun(() => {}, true);
const errPrefix = '[EV.get] ';
const { status, value: obj } = presenceSettlement(presence, errPrefix);
if (status === 'rejected') {
throw obj;
} else if (passStyleOf(obj) === 'remotable') {
throw Fail`${b(errPrefix)}Cannot read properties from remotable ${q(krefOf(obj))}`);
}
const { status: propStatus, value } = presenceSettlement(obj[key], errPrefix);
if (propStatus === 'rejected') {
throw value;
}
return value;
},
});

packages/SwingSet/tools/run-utils.ts Outdated Show resolved Hide resolved
packages/benchmark/src/benchmarkerator.js Show resolved Hide resolved
@FUDCo FUDCo force-pushed the refactor-bootstrap-test-support branch from e19d95f to 20f587d Compare October 7, 2023 02:21
@FUDCo FUDCo force-pushed the refactor-bootstrap-test-support branch 2 times, most recently from 172a839 to 0fa53ac Compare October 7, 2023 07:28
@FUDCo FUDCo force-pushed the package-kmarshal branch 2 times, most recently from db1849c to 21b185f Compare October 7, 2023 21:11
Base automatically changed from package-kmarshal to master October 7, 2023 21:56
@FUDCo FUDCo force-pushed the refactor-bootstrap-test-support branch 2 times, most recently from 984c49b to ac08b3e Compare October 10, 2023 21:25
@turadg turadg dismissed their stale review October 10, 2023 22:50

no more "test-support"

@FUDCo FUDCo force-pushed the refactor-bootstrap-test-support branch from bb821bf to 1995bde Compare October 12, 2023 17:45
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.. my humblest apologies for taking so long. One question about the dependency changes in boot.. if those aren't necessary, I'd like to remove them (and of course if they are necessary, I just want to understand why).

Lemme know if I can help with rebasing this to bring it up-to-date and fix the bitrot-induced merge conflicts.

@@ -19,9 +18,17 @@
"author": "Agoric",
"license": "Apache-2.0",
"dependencies": {
"@agoric/assert": "^0.6.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these need promotion to full dependencies, given that the benchmark (bench-vaults-performance) was moved out of this package? I kind of expect they'd be dependencies of the new packages/benchmark, but not this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alas, a bunch of stuff that I wanted to move into packages/benchmark (and which were so relocated in the original version of this PR) are just moved to the tools directory of this package. There was some controversy about where various things belong, and the easiest immediate resolution was to leave them here. I would like to revisit this in a future PR, but I don't want to hold up landing this functionality for the resolution of this matter.

@FUDCo FUDCo force-pushed the refactor-bootstrap-test-support branch from 1995bde to 3f9f8ba Compare November 18, 2023 01:02
@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label Nov 18, 2023
@mergify mergify bot merged commit 6d9ae67 into master Nov 18, 2023
77 checks passed
@mergify mergify bot deleted the refactor-bootstrap-test-support branch November 18, 2023 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge hygiene Tidying up around the house performance Performance related issues test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants