Clone + Debug on Payloads/Addresses, and compare child storage results #1203
Conversation
| @@ -42,6 +42,26 @@ pub struct StaticAddress<ReturnTy, IsDecodable> { | |||
| phantom: PhantomData<(ReturnTy, IsDecodable)>, | |||
| } | |||
|
|
|||
| impl<ReturnTy, IsDecodable> Clone for StaticAddress<ReturnTy, IsDecodable> { | |||
There was a problem hiding this comment.
Why is the manual Clone + Debug impls needed?
It should be possible to derive those I think
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
Not the prettiest code I have seen but works I guess.
I guess you can't use borrow_mut here because it can panic.
There was a problem hiding this comment.
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 :)
.github/workflows/rust.yml
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :(
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.