-
Notifications
You must be signed in to change notification settings - Fork 7
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
Improve robustness of Spacewalk integration tests #495
Improve robustness of Spacewalk integration tests #495
Conversation
It was working with `--all --all-features` but not in isolation
failed with HorizonResponseError(reqwest::Error { kind: Decode, source: Error("invalid type: string \"ySHX => SHX/XLM DAILY\", expected a borrowed string", line: 9484, column: 44) })
one test is failing with:
the address |
// and to avoid "missed" messages. | ||
latest_slot += 1; | ||
// let's wait for envelopes and txset to be available for creating a proof | ||
sleep(Duration::from_secs(5)).await; |
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.
Let's hope that this finally fixes 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.
This sleep is ultimately for poll_messages_from_stellar
process to have time for writing enough messages into ScpMessageCollector
?
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.
Yes, to build the proof.
Now the CI job failed with
which is weird because recently already changed the default fee percentile to be |
@ebma |
It might help yes. But does it make sense? Shouldn't in a perfect world our tests be able to just work with one set of accounts instead of us having to rely on switching them randomly? |
you are right. :/ |
extend timeout to 20 minutes for test_issue_execution_succeeds_from_archive
This reverts commit d5a019e.
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.
It looks good to me. Do you think you found the problem with macOS or was it more or less a coincidence that it passed this time?
// default is 90 seconds. | ||
.pool_idle_timeout(Some(Duration::from_secs(60))) | ||
// default is usize max. | ||
.pool_max_idle_per_host(usize::MAX / 2) | ||
.build()?; |
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.
Did you notice a meaningful difference with this change?
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.
To be completely honest, I am unsure.
However we encountered idle connections before and even talked about using ClientBuilder
. I guess now's the right time.
I set the pool to half of default because zero is too huge a leap.
@ebma For macos it's definitely a coincidence. |
I know I already brought this up a very long time ago, but how about we just remove the macOS job from the workflow completely? I don't see much of a benefit here, besides our workflows taking twice the time and being more likely to fail in some sense. Of course, removing something just because it doesn't work is not the ideal solution but generally, even assuming that the macOS job worked reliably all the time, I'd argue that vault operators would deploy these vaults in a Linux environment and not on a macOS machine so it should be fine to only test the workflow on Ubuntu. WDYT @b-yap? Do you want to keep it or should we remove it? |
@ebma |
Let's remove it and merge this without waiting for the checks |
Closes #494.
The multiple mainnet integration issues (and its fixes) are as follows:
DecimalsLookUp
to use.integration-test
, the clients/vault uses thestandalone-metadata
feature:spacewalk/clients/vault/src/lib.rs
Lines 52 to 53 in ea913fa
useCow<str>
insteadString
.'could not find event: Issue::ExecuteIssue'
potentially because all transactions have been traversed (reached EOF) before the memo to check hashmap was filled. A.K.A. probably race condition.IsEmptyExt
since the parameters being passed are using generic datatypes.spacewalk/clients/wallet/src/horizon/horizon.rs
Lines 255 to 257 in ea913fa