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

Merged
merged 31 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
31 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
b34bdb6
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Feb 7, 2025
73a9c4b
add test + host folder notes
usmanmani1122 Feb 7, 2025
9d67bc1
lint
usmanmani1122 Feb 7, 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 @@ -69,6 +69,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
76 changes: 49 additions & 27 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 { statSync } from 'node:fs';
import path from 'node:path';
import { parseArgs } from 'node:util';
import {
Expand Down Expand Up @@ -83,6 +84,53 @@ const prepareDockerBuild = () => {
);
};

const testProposals = () => {
const fileExists = (name: string) =>
!!statSync(name, { throwIfNoEntry: false });
usmanmani1122 marked this conversation as resolved.
Show resolved Hide resolved

// Always rebuild all test images to keep it simple. With the "use" stages
// cached, these are pretty fast building doesn't run agd.
prepareDockerBuild();

if (values.debug) {
assert(match, '--debug requires -m');
assert(proposals.length > 0, 'no proposals match');
assert(proposals.length === 1, 'too many proposals match');
const proposal = proposals[0];
console.log(chalk.yellow.bold(`Debugging ${proposal.proposalName}`));
bakeTarget(imageNameForProposal(proposal, 'test').target, values.dry);
debugTestImage(proposal);
// don't bother to delete the test image because there's just one
// and the user probably wants to run it again.
} else {
for (const proposal of proposals) {
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`, {
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' });
execSync(`docker rmi ${image.name}`, { stdio: 'inherit' });
execSync('docker system df', { stdio: 'inherit' });
}
}
};

switch (cmd) {
case 'prepare-build':
prepareDockerBuild();
Expand All @@ -98,33 +146,7 @@ switch (cmd) {
break;
}
case 'test':
// Always rebuild all test images to keep it simple. With the "use" stages
// cached, these are pretty fast building doesn't run agd.
prepareDockerBuild();
usmanmani1122 marked this conversation as resolved.
Show resolved Hide resolved

if (values.debug) {
assert(match, '--debug requires -m');
assert(proposals.length > 0, 'no proposals match');
assert(proposals.length === 1, 'too many proposals match');
const proposal = proposals[0];
console.log(chalk.yellow.bold(`Debugging ${proposal.proposalName}`));
bakeTarget(imageNameForProposal(proposal, 'test').target, values.dry);
debugTestImage(proposal);
// don't bother to delete the test image because there's just one
// and the user probably wants to run it again.
} else {
for (const proposal of proposals) {
console.log(chalk.cyan.bold(`Testing ${proposal.proposalName}`));
const image = imageNameForProposal(proposal, 'test');
bakeTarget(image.target, values.dry);
runTestImage(proposal);
// delete the image to reclaim disk space. The next build
// will use the build cache.
execSync('docker system df', { stdio: 'inherit' });
execSync(`docker rmi ${image.name}`, { stdio: 'inherit' });
execSync('docker system df', { stdio: 'inherit' });
}
}
testProposals();
break;
case 'doctor':
runDoctor(allProposals);
Expand Down
41 changes: 33 additions & 8 deletions packages/synthetic-chain/src/cli/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,53 @@ import { execSync } from 'node:child_process';
import { realpathSync } from 'node:fs';
import { ProposalInfo, imageNameForProposal } from './proposals.js';

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

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

execSync(`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
* exist.
*
* @param {typeof process.env} env environment variables
* @returns {string[]} docker run options
usmanmani1122 marked this conversation as resolved.
Show resolved Hide resolved
*/
const propagateSlogfile = env => {
const propagateSlogfile = (env: typeof process.env): string[] => {
const { SLOGFILE } = env;
if (!SLOGFILE) return [];

execSync('touch "$SLOGFILE"');
return ['-e', 'SLOGFILE', '-v', `"$SLOGFILE:${realpathSync(SLOGFILE)}"`];
return [
'--env',
'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}`;
const cmd = [
'docker',
'run',
`--network "host"`,
'--rm',
`--user "root"`,
usmanmani1122 marked this conversation as resolved.
Show resolved Hide resolved
...propagateSlogfile(process.env),
...propagateMessageFilePath(),
name,
].join(' ');
usmanmani1122 marked this conversation as resolved.
Show resolved Hide resolved
execSync(cmd, { stdio: 'inherit' });
};

Expand Down
Loading