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

Improve robustness of Spacewalk integration tests #495

Merged

Conversation

ebma
Copy link
Member

@ebma ebma commented Mar 1, 2024

Closes #494.

The multiple mainnet integration issues (and its fixes) are as follows:

  • cannot determine which DecimalsLookUp to use.
    • fix: make sure for the feature integration-test, the clients/vault uses the standalone-metadata feature:
      if #[cfg(feature = "standalone-metadata")] {
      pub type DecimalsLookupImpl = primitives::DefaultDecimalsLookup;
    • fix: make sure the clients/runtime has access to the mainnet-runtime
  • failed with
       HorizonResponseError(reqwest::Error { kind: Decode, source: Error("invalid type: string \"ySHX => SHX/XLM DAILY\", expected a borrowed string", line: 9484, column: 44) })
    
    • fix: Some memo texts have non-literal characters, messing up deserialization. A solution is to use Cow<str> instead String.
  • '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.
    • fix: DO NOT ITERATE OVER the list of transactions UNLESS the hashmap has values inside.
      • introducing IsEmptyExt since the parameters being passed are using generic datatypes.
             pub trait IsEmptyExt {
             fn is_empty(&self) -> bool;
             }
        issue_map: Arc<RwLock<T>>,
        memos_to_issue_ids: Arc<RwLock<U>>,
        filter: impl FilterWith<T, U>,

It was working with `--all --all-features` but not in isolation
@ebma ebma linked an issue Mar 1, 2024 that may be closed by this pull request
 failed with HorizonResponseError(reqwest::Error { kind: Decode, source: Error("invalid type: string \"ySHX => SHX/XLM DAILY\", expected a borrowed string", line: 9484, column: 44) })
@b-yap b-yap marked this pull request as ready for review March 7, 2024 10:04
@b-yap b-yap requested a review from a team March 7, 2024 10:04
@b-yap
Copy link
Contributor

b-yap commented Mar 8, 2024

one test is failing with:

thread 'test_redeem_succeeds_on_mainnet' panicked at 'should return ok: HorizonSubmissionError { title: "Transaction Failed", status: 400, reason: "tx_failed", result_code_op: ["op_underfunded"], envelope_xdr: Some("AAAAAgAAAACne8I5bB4KhDOubC5DCq7O/p4yvnXbjNkOE8fT3v0MHwAAAGUC7t8CAAABLwAAAAAAAAABAAAAHDNCMWdEd2U1YTZUS2dFcHVmdmJZcmRCcVZWNHYAAAABAAAAAQAAAACne8I5bB4KhDOubC5DCq7O/p4yvnXbjNkOE8fT3v0MHwAAAAEAAAAArwuPZBi/hC2gPFK8R7thoU5Xsbh+k5n1ls8DrCkdhJMAAAABVVNEQwAAAAA7mRE4Dv6Yi6CokA6xz+RPNm99vpRr7QdyQPf2JN8VxQAAAAAAAYagAAAAAAAAAAHe/QwfAAAAQElB9VXUO8NynqXVQVrdXuAECle2Iebo48ciDX+d5T0sMq3kz7YVDQqcw3uvPftSmCtRXLDoP2sAleJBIcNopQg=") }', clients/vault/tests/helper/helper.rs:196:6

the address GCTXXQRZNQPAVBBTVZWC4QYKV3HP5HRSXZ25XDGZBYJ4PU667UGB642R doesn't have any balance anymore.

// 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;
Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

clients/vault/tests/vault_integration_tests.rs Outdated Show resolved Hide resolved
clients/wallet/src/horizon/horizon.rs Show resolved Hide resolved
clients/wallet/src/horizon/responses.rs Show resolved Hide resolved
@ebma
Copy link
Member Author

ebma commented Mar 13, 2024

Now the CI job failed with

thread 'test_redeem_succeeds_on_mainnet' panicked at 'should return ok: HorizonSubmissionError { title: "Transaction Failed", status: 400, reason: "tx_insufficient_fee", result_code_op: [], 

which is weird because recently already changed the default fee percentile to be 95 instead of 90. We could set it to 100 but then we potentially pay a very large fee :(

@b-yap
Copy link
Contributor

b-yap commented Mar 13, 2024

@ebma
I think it might do us good if we have more than 2 secret keys for testing; Like 5 keys (and I meant on testnet network).
Separate keys per lib/module/package.
There's the stellar-relay-lib, wallet, vault.

@ebma
Copy link
Member Author

ebma commented Mar 13, 2024

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?

@b-yap
Copy link
Contributor

b-yap commented Mar 13, 2024

you are right. :/

Copy link
Member Author

@ebma ebma left a 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?

Comment on lines +105 to +109
// 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()?;
Copy link
Member Author

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?

Copy link
Contributor

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.

@b-yap
Copy link
Contributor

b-yap commented Mar 19, 2024

@ebma For macos it's definitely a coincidence.
I'll be keeping my eye on the mainnet tests.

@ebma
Copy link
Member Author

ebma commented Mar 19, 2024

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?

@b-yap
Copy link
Contributor

b-yap commented Mar 19, 2024

@ebma
I'd vote on removing it.

@ebma
Copy link
Member Author

ebma commented Mar 19, 2024

Let's remove it and merge this without waiting for the checks

@b-yap b-yap self-requested a review March 19, 2024 12:18
@b-yap b-yap merged commit bf6d748 into main Mar 19, 2024
1 check failed
@b-yap b-yap deleted the 494-improve-robustness-of-spacewalk-vault-integration-tests branch March 19, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve robustness of Spacewalk vault integration tests
3 participants