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

Clone + Debug on Payloads/Addresses, and compare child storage results #1203

Merged
merged 9 commits into from
Oct 18, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Oct 11, 2023

For consistency, I added proper Clone + Debug impls on the payload/address types. I also added a test for #1199 to make sure that the new batch storage method aligns with the unstable backend results, and tweaked the test stuff a bit to make it easier to add such tests.

@jsdw jsdw requested a review from a team as a code owner October 11, 2023 12:38
@@ -42,6 +42,26 @@ pub struct StaticAddress<ReturnTy, IsDecodable> {
phantom: PhantomData<(ReturnTy, IsDecodable)>,
}

impl<ReturnTy, IsDecodable> Clone for StaticAddress<ReturnTy, IsDecodable> {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the manual Clone + Debug impls needed?

It should be possible to derive those I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because Clone/Debug should be possible regardless of some of the generic params, and the default derives assume that they all impl Clone/debug too.

Buttt, I've been avoiding using derivative and maybe I should just use that instead to automate these better!

Copy link
Member

Choose a reason for hiding this comment

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

that's fine,

Do you mean that PhantomData impl differs depending what the T resolves to? I think rustc can resolve that otherwise with PhantomData but maybe I don't understand :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean that if you have:

#[derive(Clone)]
struct Foo<T> {
    val: usize,
    marker: PhantomData<T>
}

Then the Clone impl will be like:

impl <T: Clone> Clone for Foo<T> { ... }

Even though it's irrelevant whether T impls Clone or not. So really we want:

impl <T> Clone for Foo<T> { ... }

I just wrote them manually but should probably switch these to using derivative to save some loc :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I switched to using derivative to derive these things instead to get rid of all of the boilerplate!

/// Only use for comparing backends; use [`TestNodeProcess::client()`] normally,
/// which enables us to run each test against both backends.
pub async fn unstable_client(&self) -> OnlineClient<R> {
if self.unstable_client.borrow().is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

Not the prettiest code I have seen but works I guess.

I guess you can't use borrow_mut here because it can panic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was just using RefCell as a silly internal cache because Mutex wasn't needed really, so in tests you can ask for one of the clients and you'll get one back or it'll make one for you and give that back if it needs to :)

@jsdw jsdw requested a review from a team as a code owner October 18, 2023 09:54
@@ -240,6 +240,9 @@ jobs:
wasm_tests:
name: Test (WASM)
runs-on: ubuntu-latest
env:
# Set timeout for wasm tests to 120 secs; default is too low sometimes.
WASM_BINDGEN_TEST_TIMEOUT: 120
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, I do remember setting this to something like 5 minutes previously for light-clients. Didn't we have a timeout here / somewhere in the CI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't fully debugged this yet; previous runs are all failing, and locally increasing the timeout seemed to fix it but not in CI :(

@jsdw jsdw merged commit 71fbfeb into master Oct 18, 2023
10 checks passed
@jsdw jsdw deleted the jsdw-clone-debug-payloads branch October 18, 2023 21:05
@jsdw jsdw mentioned this pull request Dec 7, 2023
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.

4 participants