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

base upgrade tests on agoric-3-proposals #8476

Merged
merged 8 commits into from
Nov 8, 2023
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Oct 26, 2023

closes: #8443

Description

Passed/past upgrades are now tested in https://github.com/Agoric/agoric-3-proposals

This repo need only test the pending upgrade. The a3p tests pending core-eval proposals, but an upgrade proposal entails the whole SDK so it makes sense to test it where that code is.

This PR makes the current upgrade tests start from ghcr.io/agoric/agoric-3-proposals:main instead of ag0. That is built by,

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Improves visibility of the upgrade proposals and their tests.

Testing Considerations

Improves CI efficiency. No longer runs the entire upgrade history.
(Before: 29 to 43 min , After: maybe 7, depends on how long the disabled part takes )

Upgrade Considerations

"

@turadg turadg requested review from mhofman and raphdev October 26, 2023 17:46
Copy link
Contributor

@raphdev raphdev left a comment

Choose a reason for hiding this comment

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

Looks good to me. Agree that nightly integration is not a blocker. Linked issue discusses potential approaches for integration.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

The one regression is we won't get integration of proposals with each commit to agoric-sdk. That repo's pending proposals use agoric-sdk:dev which this repo builds on ever push so it will get those changes whenever it runs CI. That currently is only on PRs and trunk changes. We can later wire up some linking like getting-started has. Or simply have a nightly integration. Neither do I consider to be a blocker for removing the outdated tests here, but I invite the reviews to weigh in on that.

I'm not following, this dependency feels reversed to me. The way I see it, agoric-3-proposals would only contain proposals that have passed to the agoric mainnet chain. Then agoric-sdk CI would pull an image published from the agoric-3-proposals repo, and run a small integration test for every pull request simulating the next chain software upgrade. I am not sure how we would simulate core evals that are not triggered by software upgrades with that setup, but I don't think we need to solve that for now.

In general I do no want to lose the ability to run a simulated chain software upgrade from agoric-sdk CI. I actually need this capability for some upgrade-12 PRs.

@turadg turadg force-pushed the 8443-drop-upgrade-tests branch from ccbe0b8 to 5446882 Compare November 4, 2023 14:28
@turadg turadg changed the title ci: move upgrade tests to agoric-3-proposals repo base upgrade tests on agoric-3-proposals latest Nov 4, 2023
@turadg turadg requested review from mhofman and raphdev November 4, 2023 14:36
@turadg turadg added the force:integration Force integration tests to run on PR label Nov 4, 2023
@turadg turadg marked this pull request as draft November 4, 2023 15:16
@turadg turadg force-pushed the 8443-drop-upgrade-tests branch 2 times, most recently from 1ab8aba to 2430c9d Compare November 6, 2023 17:29
@turadg turadg marked this pull request as ready for review November 7, 2023 16:31
COPY ./${THIS_NAME} ./upgrade-test-scripts/${THIS_NAME}/
RUN chmod +x ./upgrade-test-scripts/*.sh
WORKDIR /usr/src/agoric-sdk/
COPY --chmod=755 ./env_setup.sh ./start_to_to.sh ./upgrade-test-scripts/
Copy link
Member

Choose a reason for hiding this comment

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

Wondering why the support scripts still need to be in this repo? I though we could get away with only having the test files themselves, and this Dockerfile (and Makefile).

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic to upgrade and then wait for upgrade is start_to_to.sh. Test files depends on env_setup.sh.

There's more cleanup that can be done but that's outside the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

The logic to upgrade and then wait for upgrade is start_to_to.sh. Test files depends on env_setup.sh.

But isn't that logic which the base image already needs?

There's more cleanup that can be done but that's outside the scope of this PR.

Of course, mostly worried about maintenance.

Copy link
Member Author

Choose a reason for hiding this comment

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

isn't that logic which the base image already needs?

Yes, for now. But I'm trying to minimize coupling. They're already at a different path from here (/usr/src/upgrade-tests-scripts/ to keep agoric-sdk path clean)

@mhofman mhofman self-requested a review November 7, 2023 18:11
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but I'd prefer if the vaults test on upgrade-12 was not disabled, as it's the only verification we have that the upgrade didn't create new failures. That said, given the simulated upgrade-12 does not actually change any contract code, I'm ok with it staying failing if needed.

@@ -27,33 +27,27 @@ test.before(async t => {
t.context.bundleIds = await installBundles(bundlesData);
});

test.serial('Open Vaults', async t => {
test.skip('Open Vaults', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

What exactly is failing in this test? It seems to be working on the current versions, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the vaults count but I don't remember for certain.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Nov 8, 2023
@turadg turadg force-pushed the 8443-drop-upgrade-tests branch from c16807b to 214667f Compare November 8, 2023 21:09
@turadg turadg changed the title base upgrade tests on agoric-3-proposals latest base upgrade tests on agoric-3-proposals Nov 8, 2023
@turadg turadg removed the force:integration Force integration tests to run on PR label Nov 8, 2023
@turadg turadg added the bypass:integration Prevent integration tests from running on PR label Nov 8, 2023
@mergify mergify bot merged commit 9b4745a into master Nov 8, 2023
81 of 83 checks passed
@mergify mergify bot deleted the 8443-drop-upgrade-tests branch November 8, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate chain upgrade tests from SDK
4 participants