-
Notifications
You must be signed in to change notification settings - Fork 214
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
ccbe0b8
to
5446882
Compare
1ab8aba
to
2430c9d
Compare
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/ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 onenv_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.
There was a problem hiding this comment.
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)
There was a problem hiding this 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 => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
c16807b
to
214667f
Compare
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
"