-
Notifications
You must be signed in to change notification settings - Fork 212
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
test(cosmic-swingset): pay 10 BLD for account with 0.25 IST to start #6187
Conversation
7d2090e
to
64b37d3
Compare
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.
Great to have this test automated!
yes... well, it would be... if it worked in ci like it does for me locally. But it doesn't, and the diagnostic doesn't give me much to go on:
I'll need help in order to advance this. |
Hi @dckc, do you want to try resurrecting this one and updating it to master? Or has its time passed? |
Let's see... yes, this is a documented feature, so it should have an automated test. As I mentioned, I'll need help. Do you have any clues for me, @michaelfig ? Here's hoping for time to look at it together. |
3c1f5e4
to
f0bc95e
Compare
after spending about 2.5 days on this last week without getting a reliable result, how about we keep the resulting test code, which I mostly like, but use |
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 about we keep the resulting test code, which I mostly like, but use
test.skip
to keep the new test out of ci? I'm pushing changes to that effect.
Works for me, but before merge I'd like to get clear on:
- Whether this is worth making work in CI eventually. If so, let's have an issue an reference it here.
- How this will be tested outside of CI. What sorts of changes should trigger someone to uncomment
.skip
and disable the other one so it's testing?
It seems like if the problem is the coordination between the two tests, we could solve that by separating them and having two chain starts. That's prohibitive for CI but maybe not for the slow tests for merging to master. Alternately we could have a script like "run this locally when you change something in the econ boot". Eventually we could have CI be smart enough to run that slow one but only when necessary.
// Sometimes I can get this test to work alone, but not | ||
// if run with the test above. |
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 tackling someday. If you agree, this would be a good place to reference an issue for it
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.
ok: #6766
That prompted me to reconsider how this is factored. I factored out It makes the change to
I'm not familiar with how the slow tests work, so I lean toward leaving that aside.
Not sure... at least you and I know about it... I think of #6766 representing the work to make sure it's part of the development process. |
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.
Refactoring looks good. Thanks for the documentation and the new issue
7082e7e
to
5efdd96
Compare
The new provision test is skipped because it's not reliable enough to run in CI, but it's useful and we'd like to keep the scenario2 refactoring work. - Makefile - note deep stack debug options - parameterize $(BLOCKS_TO_RUN) in scenario2-run-chain-to-halt - announce end of run-to-halt Makefile (SQUASHME) - factor out scenario2 shared state - only use ambient authority in test.before() - don't try to fund the provision pool with more than the PSM IST minting limit - clarify large numerals: '1234000000uist' -> `${1234e6}uist` - give names to constants such as initialHeight = 17 - document vbank/provision module address
5efdd96
to
e0e0c36
Compare
refs: #6067
Description
Integration test, including golang cosmos-sdk layers, to follow #6157
Security Considerations
?
Documentation Considerations
?
Testing Considerations
increases ava timeout to 25 min in packages/cosmic-swingset