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

feat: proposal pre/post test steps #210

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
2660fec
hooks
usmanmani1122 Jan 1, 2025
8e3f3e4
Merge branch "usman/prepare-test-hook" into branch "usman/before-afte…
usmanmani1122 Jan 1, 2025
df80864
nit
usmanmani1122 Jan 1, 2025
0554920
fixes
usmanmani1122 Jan 3, 2025
5d4e465
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Jan 3, 2025
2c94fef
document prepare-test hook
usmanmani1122 Jan 16, 2025
a689ecc
address comments
usmanmani1122 Jan 22, 2025
61371f5
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Jan 22, 2025
9f24947
increase test timeout
usmanmani1122 Jan 22, 2025
3d6780c
increase test timeout 2.0
usmanmani1122 Jan 22, 2025
5b4c029
fix for spawn
usmanmani1122 Jan 22, 2025
608e2da
:)
usmanmani1122 Jan 22, 2025
0ee2a77
increase test timeout 3.0
usmanmani1122 Jan 22, 2025
7ad7d92
revert
usmanmani1122 Jan 22, 2025
f55ef1f
address comments
usmanmani1122 Jan 23, 2025
8b22861
Empty
usmanmani1122 Jan 23, 2025
b89fe4f
Empty
usmanmani1122 Jan 23, 2025
6d5c227
Empty
usmanmani1122 Jan 23, 2025
f121f67
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Jan 24, 2025
679a083
Address comments
usmanmani1122 Jan 24, 2025
f983826
lint
usmanmani1122 Jan 24, 2025
36a2a20
use tmp directory
usmanmani1122 Jan 27, 2025
5d54940
nit
usmanmani1122 Jan 28, 2025
ef3d10b
address comments + fix bug
usmanmani1122 Jan 30, 2025
963e59f
lint
usmanmani1122 Jan 30, 2025
1a137a4
nit
usmanmani1122 Jan 31, 2025
12c5042
nit
usmanmani1122 Feb 4, 2025
ac2c07b
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Feb 4, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@turadg turadg Jan 22, 2025

Choose a reason for hiding this comment

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

🤝

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.


## Development

Expand Down
17 changes: 17 additions & 0 deletions packages/synthetic-chain/src/cli/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import chalk from 'chalk';
import assert from 'node:assert';
import { execSync } from 'node:child_process';
import { existsSync } from 'node:fs';
import path from 'node:path';
import { parseArgs } from 'node:util';
import {
Expand Down Expand Up @@ -63,6 +64,8 @@ Until https://github.com/docker/roadmap/issues/371, attempting it will error as
Instead use a builder that supports multiplatform such as depot.dev.
`;

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

/**
* Put into places files that building depends upon.
*/
Expand Down Expand Up @@ -117,7 +120,21 @@ switch (cmd) {
console.log(chalk.cyan.bold(`Testing ${proposal.proposalName}`));
const image = imageNameForProposal(proposal, 'test');
bakeTarget(image.target, values.dry);

const proposalPath = `${root}/proposals/${proposal.path}`;

if (fileExists(`${proposalPath}/pre_test.sh`))
execSync(`/bin/bash ${proposalPath}/pre_test.sh`, {
Copy link
Member

Choose a reason for hiding this comment

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

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`, {

stdio: 'inherit',
});

runTestImage(proposal);

if (fileExists(`${proposalPath}/post_test.sh`))
execSync(`/bin/bash ${proposalPath}/post_test.sh`, {
stdio: 'inherit',
});

// delete the image to reclaim disk space. The next build
// will use the build cache.
execSync('docker system df', { stdio: 'inherit' });
Expand Down
68 changes: 58 additions & 10 deletions packages/synthetic-chain/src/cli/run.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,24 @@
import { execSync } from 'node:child_process';
import { spawnSync } from 'node:child_process';
import { realpathSync } from 'node:fs';
import { ProposalInfo, imageNameForProposal } from './proposals.js';

const propagateMessageFilePath = (env: typeof process.env) => {
const fileName = 'message-file-path.tmp';
const { HOME } = env;
usmanmani1122 marked this conversation as resolved.
Show resolved Hide resolved

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@usmanmani1122 usmanmani1122 Jan 25, 2025

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

@usmanmani1122 usmanmani1122 Jan 28, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

spawnSync('touch', [filePath]);

return [
'--env',
`MESSAGE_FILE_PATH=${containerFilePath}`,
'--mount',
`source=${filePath},target=${containerFilePath},type=bind`,
];
};

/**
* Used to propagate a SLOGFILE environment variable into Docker containers.
* Any file identified by such a variable will be created if it does not already
Expand All @@ -14,17 +31,32 @@ const propagateSlogfile = env => {
const { SLOGFILE } = env;
if (!SLOGFILE) return [];

execSync('touch "$SLOGFILE"');
return ['-e', 'SLOGFILE', '-v', `"$SLOGFILE:${realpathSync(SLOGFILE)}"`];
spawnSync('touch', [SLOGFILE]);

return [
'--env',
`SLOGFILE=${SLOGFILE}`,
'--volume',
`${SLOGFILE}:${realpathSync(SLOGFILE)}`,
];
};

export const runTestImage = (proposal: ProposalInfo) => {
console.log(`Running test image for proposal ${proposal.proposalName}`);
const { name } = imageNameForProposal(proposal, 'test');
const slogOpts = propagateSlogfile(process.env);
// 'rm' to remove the container when it exits
const cmd = `docker run ${slogOpts.join(' ')} --rm ${name}`;
execSync(cmd, { stdio: 'inherit' });
spawnSync(
'docker',
[
'run',
'--network',
'host',
'--rm',
...propagateSlogfile(process.env),
...propagateMessageFilePath(process.env),
name,
],
{ stdio: 'inherit' },
);
};

export const debugTestImage = (proposal: ProposalInfo) => {
Expand All @@ -46,8 +78,24 @@ export const debugTestImage = (proposal: ProposalInfo) => {
`,
);

const slogOpts = propagateSlogfile(process.env);
// start the chain with ports mapped
const cmd = `docker run ${slogOpts.join(' ')} --publish 26657:26657 --publish 1317:1317 --publish 9090:9090 --interactive --tty --entrypoint /usr/src/upgrade-test-scripts/start_agd.sh ${name}`;
execSync(cmd, { stdio: 'inherit' });
spawnSync(
'docker',
[
'run',
'--entrypoint',
'/usr/src/upgrade-test-scripts/start_agd.sh',
'--interactive',
'--publish',
'26657:26657',
'--publish',
'1317:1317',
'--publish',
'9090:9090',
'--tty',
...propagateSlogfile(process.env),
name,
],
{ stdio: 'inherit' },
);
};
Loading