feat: proposal pre/post test steps #210

usmanmani1122 · 2025-01-01T14:38:23Z

closes: #xxxx

@agoric/synthetic-chain library

Added pre/post test steps support. This is needed to be able to run a follower in a separate container to monitor the chain under load
Enabled host network for the test container to allow communication between containers. We can also make a separate network bridge but the host network seems sufficient as we can assign different ports (than the default ones) to the follower
Added a file as volume mount from host to allow communication between containers without needing custom web servers in containers
Fixed a bug where running the build on local machine could raise a checksum error. The reason was that the node_modules were not excluded so the agops binary's symlink didn't resolve to a valid file.

A successful github action run using these changes:
https://github.com/Agoric/agoric-sdk/actions/runs/13005941935
agoric-sdk change can be seen here

…r-test-scripts"

packages/synthetic-chain/src/cli/cli.ts

packages/synthetic-chain/src/cli/run.ts

Muneeb147 · 2025-01-21T12:53:19Z

Looks good. Left some minor comments.

packages/synthetic-chain/src/cli/run.ts

packages/synthetic-chain/src/cli/cli.ts

packages/synthetic-chain/src/cli/run.ts

turadg · 2025-01-22T16:06:54Z

README.md

@@ -70,6 +70,7 @@ If the proposal is _pending_ and does not yet have a number, use a letter. The p
 - `package.json` specifies what kind of proposal it is in a `agoricProposal` field. If it's a "Software Upgrade Proposal" it also includes additional parameters.
 - `use.sh` is the script that will be run in the USE stage of the build
 - `test.sh` is the script that will be _included_ in the TEST stage of the build, and run in CI
+- `prepare-test.sh` is an optional script which can be used to run setup steps _inside_ the container


in this PR would you revise the name? Please clarify that it runs within the test image before agd starts. test-before-agd.sh would be clear.

This readme must also mention the two new hooks.

I think the hook name should not have any mention of agd. It should just be an indication of at which point in the pipeline this hook fits and prepare-test.sh I think conveys that. We could rename it to pre-test.sh as well but we then have to distinguish on hook names running outside and inside the container. The current PR uses pre-test.sh name which run on the host.
What do you suggest?

It should just be an indication of at which point in the pipeline this hook fits

Agree

and prepare-test.sh I think conveys that.

Disagree. prepare.sh works very differently,

PREPARE For upgrade proposals: submits the proposal, votes on it, runs to halt for the next stage

prepare-test.sh sounds like it would be a PREPARE step but for tests. However it runs in the same image as test.sh. It's just like test.sh but runs before agd starts.

I'll argue firmly that we need a different name than prepare-test.sh. As you point out pre and post are taken. I think those should be more explicit that they don't run within the image: before-test-run.sh and after-test-run.sh. ("Before" and "after" more closely match testing hook names like Ava has.)

That would leave pre-test.sh but that could still be easily confused. I would suggest setup-test.sh or test-before-chain.sh.

If we should discuss more let's do it synchronously or in a call.

I agree with before-test-run.sh and after-test-run.sh for host and setup-test.sh (and possibly teardown-test.sh) for image so we can lock them if you agree as well.

🤝

I like your "host" term though. I'd suggest putting the host scripts in host or some other subdir so they're not copied to the image (using --exclude). That way you can modify them without invalidating it.

turadg · 2025-01-22T16:07:57Z

packages/synthetic-chain/src/cli/cli.ts

+        const proposalPath = `${root}/proposals/${proposal.path}`;
+
+        if (fileExists(`${proposalPath}/pre_test.sh`))
+          execSync(`/bin/bash ${proposalPath}/pre_test.sh`, {


the scripts have shebang. the caller should not be concerned with how they're interpreted.

Suggested change

execSync(`/bin/bash ${proposalPath}/pre_test.sh`, {

execSync(`${proposalPath}/pre_test.sh`, {

turadg

LGTM! Good documentation.

I left one code factoring suggestion at your discretion.

turadg · 2025-01-23T17:51:10Z

packages/synthetic-chain/src/cli/cli.ts

+        fileExists(afterHookScriptPath) &&
+          execSync(afterHookScriptPath, { stdio: 'inherit' });


consider having a helper,

Suggested change

fileExists(afterHookScriptPath) &&

execSync(afterHookScriptPath, { stdio: 'inherit' });

execHostScriptIfPresent('after-test-run.sh');

Then the above three consts don't need to be defined and when someone searched after-test-run they can see easily where in the sequence it runs.

Please add running the hooks in case we're in debug mode too.

mhofman

Given the tight integration of the host hooks with run.ts it might make more sense to move their invocation there.

Also please make sure to support the debug use case as well.

mhofman · 2025-01-24T02:24:32Z

packages/synthetic-chain/src/cli/cli.ts

+        fileExists(afterHookScriptPath) &&
+          execSync(afterHookScriptPath, { stdio: 'inherit' });


Please add running the hooks in case we're in debug mode too.

mhofman · 2025-01-24T02:30:21Z

packages/synthetic-chain/src/cli/run.ts

+  const { HOME } = env;
+
+  const containerFilePath = `/root/${fileName}`;
+  const filePath = `${HOME}/${fileName}`;


I'm feeling uncomfortable with synthetic-chain hardcoding such a file, especially into the host's home dir.

Furthermore I believe this will fail when multiple test containers run concurrently.

Let's use a dynamically generated temp file, and set it in an environment variable available in the host hook scripts (I assume this is where it's needed). The container path can remain static.

Let's use a dynamically generated temp file, and set it in an environment variable available in the host hook scripts (I assume this is where it's needed). The container path can remain static.

I don't believe this was addressed. I do not want us to write anything on the host's HOME folder, especially un-prefixed. TMPDIR is fair game. I might settle for $HOME/.agoric but I really would prefer a proper tmp file that doesn't risk collisions. Also I think it'd be better to:

create the file before we start the test or call any before hook

provide the file path as an env to the spawned hooks

delete the tmp file after any spawned after hook (making sure a failure of the test or of the hook removes the file)

I will move the file to $HOME/.agoric
What kind of collision are you anticipating? The file is now prefixed with proposal name so I don’t understand. If we remove the file after the test (or before running the test) what will be the issue in that (assuming there is only a single follower running for now)?

BTW I have made the change (message file is in /tmp folder now and injected in env for the hooks)
But please add a reply to the above message for my understanding whenever you can

My concern was 2 fold:

Collisions / confusion between multiple test runs. In particular multiple tests of the same synthetic-chain test invocation. But to a lesser extent confusion about which file belonged to which invocation, if for some reason the test would not run correctly, but not sufficiently break to report a failure, and the subsequent host hook would then act on unrelated data (if the previous run had not cleaned up correctly)

garbage left behind. The purpose of these files are temporary while the test is running. We should clean them up after completion of the test. And since these are temporary in nature, they belong in the temp folder.

I agree that correctly cleaning up after the test would mitigate both the concerns, but in general I prefer to respect the semantics of the file system.

mhofman

The message file integration still feels sub-optimal.

mhofman · 2025-01-25T02:50:29Z

packages/synthetic-chain/src/cli/run.ts

+  }
+};
+
+const fileExists = (name: string) => existsSync(name);


Small use before define nit. I'm surprised we're no longer linting for this.

mhofman · 2025-01-25T02:54:36Z

packages/synthetic-chain/src/cli/run.ts

+  const { HOME } = env;
+
+  const containerFilePath = `/root/${fileName}`;
+  const filePath = `${HOME}/${fileName}`;


Let's use a dynamically generated temp file, and set it in an environment variable available in the host hook scripts (I assume this is where it's needed). The container path can remain static.

I don't believe this was addressed. I do not want us to write anything on the host's HOME folder, especially un-prefixed. TMPDIR is fair game. I might settle for $HOME/.agoric but I really would prefer a proper tmp file that doesn't risk collisions. Also I think it'd be better to:

create the file before we start the test or call any before hook

provide the file path as an env to the spawned hooks

delete the tmp file after any spawned after hook (making sure a failure of the test or of the hook removes the file)

mhofman

Thanks for making the switch to a tmp file. Let's make sure things are cleaned up, and that we use less manual naming of the tmp file.

mhofman · 2025-01-29T19:47:59Z

packages/synthetic-chain/src/cli/run.ts

 import { ProposalInfo, imageNameForProposal } from './proposals.js';

+const createMessageFile = () => {
+  const messageFileName = `${new Date().getTime()}.tmp`;
+  const messageFilePath = `/tmp/${messageFileName}`;


At the very least this should use tmpdir, but I would even prefer to use tmp. That package also makes removal easier. Finally, I did prefer when the proposal name was included in the tmp file name, it makes debugging easier.

I wasn't sure adding a dependency of tmp package was really needed here
Added

mhofman · 2025-01-29T19:52:22Z

packages/synthetic-chain/src/cli/run.ts

+  process.env.MESSAGE_FILE_PATH = messageFilePath;
+
+  executeHostScriptIfPresent(proposal, 'before-test-run.sh');


I think it'd be preferable to plumb the messageFilePath into executeHostScriptIfPresent and for that spawn to use the modified env of the child process only. Mutating the current process env goes against best practices.

You can either spread the current env, or use inheritance ({__proto__: process.env, MESSAGE_FILE_PATH: messageFilePath})

mhofman · 2025-01-29T19:53:18Z

packages/synthetic-chain/src/cli/run.ts

 };

 export const debugTestImage = (proposal: ProposalInfo) => {
+  executeHostScriptIfPresent(proposal, 'before-test-run.sh');


To clarify, we don't expect tests run in debug mode to have a message file? Is there not a risk that host scripts would expect a message file and break if not available?

Valid point
Done

mhofman · 2025-01-29T20:02:50Z

packages/synthetic-chain/src/cli/run.ts

+  const { HOME } = env;
+
+  const containerFilePath = `/root/${fileName}`;
+  const filePath = `${HOME}/${fileName}`;


My concern was 2 fold:

Collisions / confusion between multiple test runs. In particular multiple tests of the same synthetic-chain test invocation. But to a lesser extent confusion about which file belonged to which invocation, if for some reason the test would not run correctly, but not sufficiently break to report a failure, and the subsequent host hook would then act on unrelated data (if the previous run had not cleaned up correctly)

garbage left behind. The purpose of these files are temporary while the test is running. We should clean them up after completion of the test. And since these are temporary in nature, they belong in the temp folder.

I agree that correctly cleaning up after the test would mitigate both the concerns, but in general I prefer to respect the semantics of the file system.

mhofman

I like the new generate copy command.

I had missed that a previous commit changed the network bind to use the host interfaces, any reason for that change?

mhofman · 2025-01-31T02:38:40Z

packages/synthetic-chain/src/cli/run.ts

+}) => {
+  const { name: messageFilePath, removeCallback: removeTempFileCallback } =
+    createMessageFile(proposal);
+  const messageFileName = basename(messageFilePath);

  const containerFilePath = `/root/${messageFileName}`;


I thought we would have the message file in the container use a static name (not even related to the proposal name) to avoid having to plumb an extra env.

Container file doesn't necessarily need to use the proposal name in it's path, but we still need to pass the host file path to the host start script so that the follower can mount that file in it's container

Right but the code above will make the container file name dynamic, without any way for the container script to figure out what that name is. Or am I missing something?

mhofman · 2025-01-31T02:39:59Z

packages/synthetic-chain/src/cli/run.ts

+        `source=${messageFilePath},target=${containerFilePath},type=bind`,
+        '--network',
+        'host',
+        removeContainerOnExit && '--rm',


To avoid the .filter(Boolean).map(String):

Suggested change

removeContainerOnExit && '--rm',

...(removeContainerOnExit ? ['--rm'] : []),

packages/synthetic-chain/src/cli/run.ts

mhofman · 2025-01-31T02:43:24Z

packages/synthetic-chain/src/cli/run.ts

-      '--publish',
-      '1317:1317',
-      '--publish',
-      '9090:9090',
-      '--publish',
-      '26657:26657',


Why are we losing these port mappings?

Because of the host network these will still be available

mhofman · 2025-01-31T02:47:51Z

packages/synthetic-chain/src/cli/run.ts

+        '--network',
+        'host',


I'm pretty sure this is a bad idea. Why do we need to hook onto the host network interfaces?

If we don't use host network, we would need individual container IPs
Using host network will make the RPC/Peer available on localhost (using different ports)

I don't understand. How is different IPs different than different ports?
Also afaik, only the acceptance test needs to be connected to, so couldn't we only target that container?

hooks

2660fec

usmanmani1122 self-assigned this Jan 1, 2025

usmanmani1122 added 4 commits January 1, 2025 15:26

Merge branch "usman/prepare-test-hook" into branch "usman/before-afte…

8e3f3e4

…r-test-scripts"

nit

df80864

fixes

0554920

Merge branch 'main' into usman/before-after-test-scripts

5d4e465

usmanmani1122 changed the title ~~hooks~~ feat: proposal pre/post test steps Jan 3, 2025

usmanmani1122 requested a review from mhofman January 3, 2025 12:57

usmanmani1122 marked this pull request as ready for review January 3, 2025 13:00

document prepare-test hook

2c94fef

Muneeb147 reviewed Jan 21, 2025

View reviewed changes

packages/synthetic-chain/src/cli/cli.ts Outdated Show resolved Hide resolved

packages/synthetic-chain/src/cli/run.ts Outdated Show resolved Hide resolved

Muneeb147 reviewed Jan 21, 2025

View reviewed changes

packages/synthetic-chain/src/cli/run.ts Outdated Show resolved Hide resolved

usmanmani1122 requested a review from Muneeb147 January 21, 2025 14:35

turadg reviewed Jan 21, 2025

View reviewed changes

usmanmani1122 added 8 commits January 22, 2025 11:02

address comments

a689ecc

Merge branch 'main' into usman/before-after-test-scripts

61371f5

increase test timeout

9f24947

increase test timeout 2.0

3d6780c

fix for spawn

5b4c029

:)

608e2da

increase test timeout 3.0

0ee2a77

revert

7ad7d92

usmanmani1122 requested a review from turadg January 22, 2025 11:28

turadg reviewed Jan 22, 2025

View reviewed changes

usmanmani1122 added 4 commits January 23, 2025 15:41

address comments

f55ef1f

Empty

8b22861

Empty

b89fe4f

Empty

6d5c227

turadg approved these changes Jan 23, 2025

View reviewed changes

Muneeb147 approved these changes Jan 23, 2025

View reviewed changes

mhofman requested changes Jan 24, 2025

View reviewed changes

usmanmani1122 added 2 commits January 24, 2025 14:41

Merge branch 'main' into usman/before-after-test-scripts

f121f67

Address comments

679a083

usmanmani1122 enabled auto-merge (squash) January 24, 2025 13:10

lint

f983826

usmanmani1122 requested a review from mhofman January 24, 2025 13:13

mhofman reviewed Jan 25, 2025

View reviewed changes

usmanmani1122 added 2 commits January 27, 2025 17:30

use tmp directory

36a2a20

nit

5d54940

usmanmani1122 requested a review from mhofman January 28, 2025 10:13

mhofman reviewed Jan 29, 2025

View reviewed changes

usmanmani1122 added 2 commits January 30, 2025 18:22

address comments + fix bug

ef3d10b

lint

963e59f

usmanmani1122 requested a review from mhofman January 30, 2025 13:56

mhofman reviewed Jan 31, 2025

View reviewed changes

mhofman requested a review from turadg January 31, 2025 02:51

nit

1a137a4

usmanmani1122 requested a review from mhofman January 31, 2025 07:53

usmanmani1122 added 2 commits February 4, 2025 18:49

nit

12c5042

Merge branch 'main' into usman/before-after-test-scripts

ac2c07b

	execSync(`/bin/bash ${proposalPath}/pre_test.sh`, {
	execSync(`${proposalPath}/pre_test.sh`, {

		fileExists(afterHookScriptPath) &&
		execSync(afterHookScriptPath, { stdio: 'inherit' });

	fileExists(afterHookScriptPath) &&
	execSync(afterHookScriptPath, { stdio: 'inherit' });
	execHostScriptIfPresent('after-test-run.sh');

		process.env.MESSAGE_FILE_PATH = messageFilePath;

		executeHostScriptIfPresent(proposal, 'before-test-run.sh');

	removeContainerOnExit && '--rm',
	...(removeContainerOnExit ? ['--rm'] : []),

feat: proposal pre/post test steps #210

Are you sure you want to change the base?

feat: proposal pre/post test steps #210

Conversation

usmanmani1122 commented Jan 1, 2025 • edited Loading

@agoric/synthetic-chain library

Muneeb147 commented Jan 21, 2025

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 Jan 22, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman 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

usmanmani1122 Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

usmanmani1122 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman 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

mhofman 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

usmanmani1122 commented Jan 1, 2025 •

edited

Loading

turadg Jan 22, 2025 •

edited

Loading

usmanmani1122 Jan 25, 2025 •

edited

Loading

usmanmani1122 Jan 28, 2025 •

edited

Loading