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
Added a file as volume mount from host to allow communication between containers without needing custom web servers in containers

…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

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?

I never submitted this last week, but for future reference:

We can get the IP address from inside the container using something like hostname -i (trimming trailing space). We already need to get the tendermint node-id and probably should get the chain_id (from genesis file). The setup test script can grab those and send over the message file.

Muneeb147 · 2025-02-06T13:12:18Z

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

+      { stdio: 'inherit' },
+    );
+
+    executeHostScriptIfPresent(


So at this point our tests are being executed inside container, right?

turadg

Thanks for restoring the ports config @usmanmani1122 .

Before merging please update the PR description and clean up the commits. I think there are two now:

feat: proposal pre/post test steps
fix: exclude node_modules from COPY

(Incidentally, we couldn't exclude node_modules until Docker added the experimental exclude feature)

mhofman

Thanks for putting all this in. I second @turadg's request for cleaning up the commits before merging. This PR now does multiple things and I would prefer it not to be squashed, but interactively rebased to tell a clear story (or split into multiple squashed PRs if you prefer).

mhofman · 2025-02-06T19:38:13Z

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

+  const { name: messageFilePath, removeCallback: removeTempFileCallback } =
+    createMessageFile(proposal);
+
+  const containerFilePath = '/root/message-file-path';


Nit:

Suggested change

const containerFilePath = '/root/message-file-path';

const containerFilePath = '/root/host-test-message';

mhofman · 2025-02-06T19:44:18Z

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

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


I never submitted this last week, but for future reference:

We can get the IP address from inside the container using something like hostname -i (trimming trailing space). We already need to get the tendermint node-id and probably should get the chain_id (from genesis file). The setup test script can grab those and send over the message file.

usmanmani1122 · 2025-02-07T05:54:31Z

This PR now does multiple things and I would prefer it not to be squashed, but interactively rebased to tell a clear story (or split into multiple squashed PRs if you prefer).

I think it would be better to create a separate PR for the exclude node_modules fix as there isn't a single commit with just that change

Muneeb147 · 2025-02-07T10:32:40Z

Request for cleaning up the commits before merging. This PR now does multiple things and I would prefer it not to be squashed, but interactively rebased to tell a clear story (or split into multiple squashed PRs if you prefer).

@usmanmani1122 So what do we plan? Cleaning commits or separate PRs?
@frazarshad Can help in cleaning commits with interactive mode of git.

usmanmani1122 · 2025-02-07T10:33:33Z

@usmanmani1122 So what do we plan? Cleaning commits or separate PRs? @frazarshad Can help in cleaning commits with interactive mode of git.

The node_modules change is merged separately. This PR can be squashed and merged now

usmanmani1122 · 2025-02-07T10:35:37Z

Thanks for restoring the ports config @usmanmani1122 .

Before merging please update the PR description and clean up the commits. I think there are two now:

feat: proposal pre/post test steps

fix: exclude node_modules from COPY

(Incidentally, we couldn't exclude node_modules until Docker added the experimental exclude feature)

@turadg I have merged the node_modules exclusion change separately so this PR includes the relevant changes only

turadg · 2025-02-07T18:31:47Z

README.md

+- `before-test-run.sh` is an optional script which can be used to run setup steps on the _host_ (like starting a follower). It runs _before_ the container launches. It needs to be placed in the **host** folder.
+- `after-test-run.sh` is an optional script which can be used to run teardown steps on the _host_ (like stopping a follower). It runs _after_ the container exits. It needs to be placed in the **host** folder.


Let's be more explicit about what the host folder is for. There has been some confusion: Agoric/agoric-sdk#10947 (comment)

Suggested change

- `before-test-run.sh` is an optional script which can be used to run setup steps on the _host_ (like starting a follower). It runs _before_ the container launches. It needs to be placed in the **host** folder.

- `after-test-run.sh` is an optional script which can be used to run teardown steps on the _host_ (like stopping a follower). It runs _after_ the container exits. It needs to be placed in the **host** folder.

- `host` folder is for ___

- `host/before-test-run.sh` is an optional script which can be used to run setup steps on the _host_ (like starting a follower). It runs _before_ the container launches.

- `host/after-test-run.sh` is an optional script which can be used to run teardown steps on the _host_ (like stopping a follower). It runs _after_ the container exits.

While you're at it a change like this would help,

- `test` folder is for files that should be included only in the `test` image

This is not blocking feedback. If something is delayed by this PR the docs improvements can come later.

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

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

Muneeb147 reviewed Feb 6, 2025

View reviewed changes

turadg requested changes Feb 6, 2025

View reviewed changes

mhofman approved these changes Feb 6, 2025

View reviewed changes

usmanmani1122 disabled auto-merge February 7, 2025 05:45

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

b34bdb6

usmanmani1122 enabled auto-merge (squash) February 7, 2025 10:32

usmanmani1122 requested a review from turadg February 7, 2025 10:34

turadg reviewed Feb 7, 2025

View reviewed changes

turadg mentioned this pull request Feb 7, 2025

Repair u19 tests in a3p-integration Agoric/agoric-sdk#10947

Merged

usmanmani1122 and others added 2 commits February 8, 2025 00:59

add test + host folder notes

73a9c4b

lint

9d67bc1

usmanmani1122 requested a review from turadg February 7, 2025 20:03

turadg approved these changes Feb 7, 2025

View reviewed changes

usmanmani1122 merged commit 80b3987 into main Feb 7, 2025
2 checks passed

usmanmani1122 deleted the usman/before-after-test-scripts branch February 7, 2025 20:26

	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'] : []),

	const containerFilePath = '/root/message-file-path';
	const containerFilePath = '/root/host-test-message';

		- `before-test-run.sh` is an optional script which can be used to run setup steps on the _host_ (like starting a follower). It runs _before_ the container launches. It needs to be placed in the host folder.
		- `after-test-run.sh` is an optional script which can be used to run teardown steps on the _host_ (like stopping a follower). It runs _after_ the container exits. It needs to be placed in the host folder.

-- `before-test-run.sh` is an optional script which can be used to run setup steps on the _host_ (like starting a follower). It runs _before_ the container launches. It needs to be placed in the **host** folder.
-- `after-test-run.sh` is an optional script which can be used to run teardown steps on the _host_ (like stopping a follower). It runs _after_ the container exits. It needs to be placed in the **host** folder.
+- `host` folder is for ___
+  - `host/before-test-run.sh` is an optional script which can be used to run setup steps on the _host_ (like starting a follower). It runs _before_ the container launches.
+  - `host/after-test-run.sh` is an optional script which can be used to run teardown steps on the _host_ (like stopping a follower). It runs _after_ the container exits.

feat: proposal pre/post test steps #210

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment • edited Loading

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

usmanmani1122 commented Feb 7, 2025 • edited Loading

Muneeb147 commented Feb 7, 2025

usmanmani1122 commented Feb 7, 2025

usmanmani1122 commented Feb 7, 2025

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

turadg left a comment •

edited

Loading

usmanmani1122 commented Feb 7, 2025 •

edited

Loading