Porting upgrade tests to Javascript #8145

iomekam · 2023-08-04T20:57:40Z

Description

Refactoring our bash scripts to look and feel more like Javascript. To accomplish portions of the scripts shelling out, we've leveraged. We intend to port all the upgrade tests, but started this endeavor with agoric-upgrade-10

To discuss a few of the changes:

pre_test.sh, actions.sh, and test.sh have been moved. respectfully, to pre.test.js, actions.test.js, and post.test.js.
We are now using AVA as the test runner. To run the upgrade tests, you can run yarn upgrade-test within the upgrade-test-scripts folder
We've added a few helper files to make writing tests a little less cumbersome. These helpers have replaced the functionality of env_setup.sh

Chris-Hibbert · 2023-08-16T20:27:18Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+      if (run === 1) {
+        try {
+          const params = ['ec', 'committee', '--send-from', account];
+          await executeCommand(agopsLocation, params, {


This seems to be doing almost the same thing as line 47. Why is the syntax so different?

Was there a missed opportunity to turn line 39 into the agops.ec() form, or is this intended to demonstrate the two styles of call?

Ah good callout. At one point, there was a reason why I had to do it this way (to pass in custom options to execa), but I moved away from that approach, so I can converge this to look like the call below

Actually, on second thought, the reason I did this was to pass in the timeout option to execa

Chris-Hibbert · 2023-08-16T20:31:47Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+        }
+      } else {
+        try {
+          await agops.ec('committee', '--send-from', account, '--voter', voter);


It seems like if we're going to add helpers for agops subcommands, we could turn them into actual javascript calls rather than requiring the user to mimic the CLI version. It seems like the flags could be omitted, so the call here could be agops.ec(account, voter);. [I don't know whether 'committee' is a flag or not. If the user has to specify the committee name, for instance, then that could be an extra parameter, and the javascript function's signature would help the caller know what arguments to provide.

Chris-Hibbert · 2023-08-16T20:34:46Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+    return agops.auctioneer(
+      'proposeParamChange',
+      '--charterAcceptOfferId',
+      charterAcceptOfferId,
+      '--start-frequency',
+      startFequency,
+      '--clock-step',
+      clockStep,
+      '--price-lock-period',
+      priceLockPeriod,
+    );
+  };


This doesn't seem like an improvement over the shell syntax. Can we convert to JS style call? Would more work in the agops commands make this easier to support?

FASTER_AUCTIONS_OFFER=$(mktemp -t agops.XXX) agops auctioneer proposeParamChange --charterAcceptOfferId "$(agops ec find-continuing-id --for "charter member invitation" --from "$GOV1ADDR")" --start-frequency $START_FREQUENCY --clock-step $CLOCK_STEP --price-lock-period $PRICE_LOCK_PERIOD >|"$FASTER_AUCTIONS_OFFER" agoric wallet print --file "$FASTER_AUCTIONS_OFFER" agops perf satisfaction --from "$GOV1ADDR" --executeOffer "$FASTER_AUCTIONS_OFFER" --keyring-backend=test

dckc · 2023-08-16T20:47:21Z

before I get into detailed feedback...

Getting all this working in ci is great work!

turadg

Love it!

I left some comments and requests. Happy to hop on a call to go over them.

packages/deployment/upgrade-test/Dockerfile

turadg · 2023-08-16T20:37:51Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

@@ -0,0 +1,348 @@
+/* eslint-disable @jessie.js/safe-await-separator */


remark: we should eventually update our configs so this suppression isn't necessary

Here's the blurb explaining why it was setup in the first case. Is this no longer relevant?

It's necessary for contract code. It's not a risk for deployment upgrade test code.

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

turadg · 2023-08-16T20:41:11Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+  let voter = 0;
+  for (const account of govAccounts) {
+    const runs = [1, 2];
+    for (const run of runs) {


a little funny to have a loop where most of the lines are conditional on the index.

import: refactor to a sequence of calls.
suggestion: helper function for doing whatever is in common

turadg · 2023-08-16T20:42:33Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+    const offerFunction = async () => {
+      return agops.oracle('accept', '--offerId', oracle.id);
+    };


the function is returning a promise but it's not async.

Suggested change

const offerFunction = async () => {

return agops.oracle('accept', '--offerId', oracle.id);

};

const offerFunction = () => agops.oracle('accept', '--offerId', oracle.id);

turadg · 2023-08-16T20:54:31Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.test.js

+  await submitDeliverInbound('user1');
+
+  const oracles = [];
+  const oraclesAddresses = [GOV1ADDR, GOV2ADDR];


important: hoist

turadg · 2023-08-16T20:55:21Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.test.js

+  );
+
+  const govParams = await agoric.follow('-lF', ':published.auction.governance');
+  t.is(govParams.current.ClockStep.value.relValue, CLOCK_STEP.toString());


that relvalue is a string makes me think the unmarshaler needs work.

Maybe we should file an issue to get this into the backlog?

pretty sure it's a byproduct of the open coded unmarshaller

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/legacy.sh

turadg · 2023-08-16T20:57:45Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/post.test.js

+import { GOV1ADDR, GOV2ADDR, GOV3ADDR, USER1ADDR } from '../constants.mjs';
+import { calculateWalletState } from './upgradeHelpers.mjs';
+
+test.serial(


in post.test.js the tests needn't be serial because they're not supposed to affect the chain

important: remove serial in test that don't depend on it

turadg · 2023-08-16T21:01:06Z

packages/deployment/upgrade-test/upgrade-test-scripts/cliHelper.mjs

+  cwd: '/usr/src/agoric-sdk',
+})`yarn bin agops`;
+
+export const agops = {


remark: I worry we're creating more work for ourselves by going from JS to shell and shell back to JS. This test is of upgrade . We don't need to integrate with Commander.js. We might better off importing the modules from agoric-cli directly. We'd get type checking that way!

dckc

just flushing a pending comment that I didn't mean to make pending
I don't understand what github is doing here

dckc · 2023-08-16T20:45:31Z

packages/deployment/upgrade-test/upgrade-test-scripts/package.json

@@ -0,0 +1,20 @@
+{
+  "type": "module",


Since we've got type module here, we use .js instead of .mjs?

dckc · 2023-08-16T21:19:01Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+  const deadline = toSec(Date.now()) + voteDurSec;
+
+  const zip = (xs, ys) => xs.map((x, i) => [x, ys[i]]);
+  const fromSmallCapsEntries = txt => {


all the smallcaps manipulations in the JS thingy I wrote are to avoid using anything outside the node standard library. Since we've now crossed over into using package.json and such, this should switch to using @endo/marshal in the normal way.

Chris-Hibbert · 2023-08-16T21:51:41Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+  await waitForBlock();
+};
+
+export const ensureNewAuctionParams = async (


Isn't this more like implementNewAuctionParams? The test that calls does the ensure part, I think.

(all the other verbs I can think of are too overloaded: you "pass" a resolution. Maybe 'enact'?)

Chris-Hibbert · 2023-08-16T21:56:14Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.mjs

+      address,
+    );
+
+    return paramChangeOfferGeneration(charterAcceptOfferId, 30, 123000000);


Seems like the new value should be a parameter. That way the test 'Ensure debt ceiling raised' could verify that the passed value matches the proposal.

This may be refactored entirely, as stated here

turadg

There's more work to do before closing out #8137 but this is definite progress that I think is worth landing.

We have an urgent need to write new tests and this will allow us to write them in JS. We can always improve the code quality and DX incrementally.

turadg · 2023-08-17T19:35:48Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-10/actions.test.js

+  );
+
+  const govParams = await agoric.follow('-lF', ':published.auction.governance');
+  t.is(govParams.current.ClockStep.value.relValue, CLOCK_STEP.toString());


pretty sure it's a byproduct of the open coded unmarshaller

Porting upgrade tests to Javascript

iomekam force-pushed the execa-proto branch from 3c42842 to 2a2085c Compare August 15, 2023 22:27

iomekam changed the title ~~Bash -> Execa~~ Moving our upgrade tests to Javascript Aug 15, 2023

iomekam changed the title ~~Moving our upgrade tests to Javascript~~ Porting upgrade tests to Javascript Aug 15, 2023

iomekam force-pushed the execa-proto branch from a76693b to bf64809 Compare August 16, 2023 19:33

iomekam marked this pull request as ready for review August 16, 2023 19:34

iomekam requested review from dckc, turadg and Chris-Hibbert August 16, 2023 19:34

Chris-Hibbert reviewed Aug 16, 2023

View reviewed changes

turadg requested changes Aug 16, 2023

View reviewed changes

dckc reviewed Aug 16, 2023

View reviewed changes

Chris-Hibbert reviewed Aug 16, 2023

View reviewed changes

iomekam requested a review from turadg August 17, 2023 19:30

turadg approved these changes Aug 17, 2023

View reviewed changes

feat: port bash tests to javascript

84f2e6f

iomekam force-pushed the execa-proto branch from ca8feab to 84f2e6f Compare August 17, 2023 21:23

iomekam added this pull request to the merge queue Aug 17, 2023

Merged via the queue into master with commit a3d49e5 Aug 17, 2023

iomekam deleted the execa-proto branch August 17, 2023 22:28

mhofman pushed a commit that referenced this pull request Nov 8, 2023

Merge pull request #8145 from Agoric/execa-proto

53ef584

Porting upgrade tests to Javascript

mhofman pushed a commit that referenced this pull request Nov 10, 2023

Merge pull request #8145 from Agoric/execa-proto

fcee255

Porting upgrade tests to Javascript

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting upgrade tests to Javascript #8145

Porting upgrade tests to Javascript #8145

iomekam commented Aug 4, 2023 •

edited

Loading

Chris-Hibbert Aug 16, 2023

iomekam Aug 16, 2023

iomekam Aug 17, 2023

Chris-Hibbert Aug 16, 2023

Chris-Hibbert Aug 16, 2023

dckc commented Aug 16, 2023

turadg left a comment

turadg Aug 16, 2023

iomekam Aug 17, 2023

turadg Aug 17, 2023

turadg Aug 16, 2023

turadg Aug 16, 2023

turadg Aug 16, 2023

turadg Aug 16, 2023

iomekam Aug 17, 2023

turadg Aug 17, 2023

turadg Aug 16, 2023

turadg Aug 16, 2023

dckc left a comment

dckc Aug 16, 2023

dckc Aug 16, 2023

Chris-Hibbert Aug 16, 2023

Chris-Hibbert Aug 16, 2023

iomekam Aug 17, 2023

turadg left a comment

turadg Aug 17, 2023

		@@ -0,0 +1,348 @@
		/* eslint-disable @jessie.js/safe-await-separator */

Porting upgrade tests to Javascript #8145

Porting upgrade tests to Javascript #8145

Conversation

iomekam commented Aug 4, 2023 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Aug 16, 2023

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iomekam commented Aug 4, 2023 •

edited

Loading