Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Repair u19 tests in a3p-integration #10947
base: master
Are you sure you want to change the base?
Repair u19 tests in a3p-integration #10947
Changes from 6 commits
750a7f2
5af83d0
b068711
af22f09
f7dfbac
420220f
5576d7e
d628279
17f2c0d
730d4f5
f302496
d89dde5
1116aa9
ab93821
cc546bd
82ab9eb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 155 in a3p-integration/proposals/n:upgrade-next/agoricNames.test.js
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.
how did this number go down?
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.
I don't know for sure how the test passed with this number. My guess would be that the code last ran in an environment where a Zoe upgrade was included with U18 or the proto U19.
The lower number is correct following U18, and in a U19 that doesn't upgrade Zoe.
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.
I think because
upgrade.go
somehow kept some u18 proposals around when it was rebased on top of u18.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.
seems useful but you'd know better since you've been debugging
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.
nit: "test" is already a verb
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.
Hmm. I read the string argument as the name of the test, rather than a noun phrase describing what is to be tested. In this case the test used to perform the upgrade (because the proposal was a coreEval), and now only verifies it (because it's now a software upgrade).
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.
all these proposals for testing should be under
test/
so they don't invalidate the "use" image. see,Making that change affects .gitignore and .tsconfig but if you put them under a path like
test/submissions
then you can use that pathname for those files and not have to manage a list of submissions.We'll make this easier eventually,
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.
Actually they should be in
host
, and their core eval output should be intest
Something like
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.
FYI @Chris-Hibbert , the
host
thing hasn't landed in a3p. Up to you whether to wait for it to be merged and released to NPM so you can include that synthetic-chain lib version in 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.
I like your
test/generated
approach. It still copies to the image unnecessarily, but it's only the test image so it doesn't invalid any extra. (If the proposal changes its build would anyway). I think it's better to keep test stuff together than to spread it into another path.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.
Yeah you're right, a change in the proposal building would likely invalidate everything anyway, but collocating test files together is cleaner.
I got confused about the
host
stuff however, I forgot these first argument is relative to the SDK packages.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.
I won't block on this, but we've been abusing
test.serial
. Its only guarantee is "not serial" and we've been relying on it being sequential.Consider factoring into one
test()
call. If the function is unwieldy you can use a driver model to abstract the steps. Or just put a log message for each stage.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.
You mean "not concurrent". I honestly didn't realize it didn't guarantee sequential.
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.
Yeah thanks for correcting my write-o.
https://github.com/avajs/ava/blob/main/docs/01-writing-tests.md#running-tests-serially
avajs/ava#3238
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.