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

testing: Prepare light client testing with substrate binary and add subxt-test macro #1507

Merged
merged 44 commits into from
Apr 8, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Mar 28, 2024

In this PR

  • subxt-macro is added. The macro runs the tests with a default timeout (6 minutes if not specified)
  • added cfg aliases for our testing with a new unstable-light-client-long-running feature flag to reuse subxt testing with the lightclient
  • some tests are disabled in the context of the light-client
  • chainHead initialized event supports the legacy format (to make it work with our outdated lightclient version)
  • small testing refactoring to decouple the construction of transaction from the submit operation (might still be needed in the future if we want to retry failed light-client operations)

Builds on top of: #1459 (because the CI was not running anymore)

Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv requested review from a team as code owners March 28, 2024 11:45
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@@ -0,0 +1,20 @@
[package]
name = "subxt-test-proc-macro"
Copy link
Collaborator

@jsdw jsdw Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: also rename the folder to subxt-test-proc-macro (or subxt-test-macro if you prefer to shorten this crate name)?

}
}

submit().await
Copy link
Collaborator

@jsdw jsdw Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if using the light client, we try submitting a couple of additional times to handle light client specific issues?

I wonder if there's anything we can do to avoid needing to do this; does needing it imply that using the light client to submit transactions now is not that robust and will fail sometimes? Is there anything we can do before we try submitting that might help things (ie wait a few blocks), so that by the time we submit we don't need anything special? Is this just because of the legacy backend and everything works ok with the unstable backend (and in that case I think I'd only try testing the unstable backend with the light client)?

If we do need to keep this function then I think it'd be worth adding some docs to explain why it's used for future us :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that makes sense. I've removed the tx_retries for now.
Instead maybe we should report those tx::Dropped events to smoldot if they still happen after an upgrade.

@lexnv lexnv changed the title testing: Test light client with substrate binary testing: Prepare light client testing with substrate binary and add subxt-test macro Apr 3, 2024
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Is the effect of removing that function that some tests will fail sometimes now when using the light client?

@lexnv
Copy link
Collaborator Author

lexnv commented Apr 3, 2024

When we run tests with the unstable-light-client-long-running we'll probably encounter more flaky failures to due TransactionEvent::Dropped. However, that is not enabled now in the CI and I would generally wait to enable that until we have: #1400.

I'll look into that issue shortly, hopefully we could handle that and create a PR in smoldot if the fix is not too involved

}

let ir = InitializedIR::deserialize(deserializer)?;
let finalized_block_hashes = ir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly weird comment; we could actually throw an error if both finalized_block_hash and finalized_block_hashes.

now, we checking finalized_block_hashes first and if that doesn't exist we fallback to finalized_block_hash which is probably fine but if one inserts both by accident it lead to confusing scenarios but fine :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this only exists until we update smoldot :)


#[cfg(fullclient)]
use crate::utils::node_runtime;
#[cfg(fullclient)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to merge the use stuff to avoid repeating #[cfg(fullclient)]?

const SUBXT_TEST_TIMEOUT: &str = "SUBXT_TEST_TIMEOUT";

/// Default timeout for the test.
const DEFAULT_TIMEOUT: u64 = 60 * 6;
Copy link
Member

@niklasad1 niklasad1 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nicer/more readable to use Duration here IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would still need to pass std::time::Duration to the quote! below, which does not impl toTokens , I've renamed this to illustrate better that we are counting seconds, thanks!

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, I really like this

Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv merged commit b31131d into master Apr 8, 2024
13 checks passed
@lexnv lexnv deleted the lexnv/light-client-testing-macro branch April 8, 2024 08:34
@jsdw jsdw mentioned this pull request May 16, 2024
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.

None yet

3 participants