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

statement-distribution: Add tests for incoming acknowledgements #2498

Merged
merged 9 commits into from
Dec 5, 2023

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Nov 26, 2023

In #2329 we noticed that some async backing functions were missing tests. This adds a few tests for receiving acknowledgements. More tests will come in a future PR.

I also started doing the refactor described in #2382, plus some more refactor ideas I had. It would have made this PR way too big to finish this refactor, so I only did it for the new tests. If it looks good, I'll raise another PR refactoring all the tests. Personally I think it makes the tests much easier to read, write, and reason about. For me it's important because I read the tests to find out how async backing is expected to behave in certain situations.

Related

See #2329
See #2382

@mrcnski mrcnski self-assigned this Nov 26, 2023
@mrcnski mrcnski requested review from ordian and removed request for rphmeier December 1, 2023 13:41
@mrcnski
Copy link
Contributor Author

mrcnski commented Dec 4, 2023

@AndreiEres brought up a good point about these asserts:

assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await;
assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await;
assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await;
assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE).await;

If one of the asserts here fails, the error will point to inside the assert function and not where the assert is called in code (what we care about). Actually this is an existing issue with these tests and it's annoyed me before.

I believe we could fix it by making these asserts macros instead of functions. Indeed, I just tested it and it works.

tests/grid.rs:

assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT);
assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await;
assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await;
assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE).await;

tests/mod.rs:

#[macro_export]
macro_rules! assert_peer_reported {
    ($virtual_overseer:expr, $peer_id:expr, $rep_change:expr) => {
        assert!(false);
        assert_matches!(
            $virtual_overseer.recv().await,
            AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r)))
                if p == $peer_id && r == $rep_change.into()
        );
    }
}

Result:

thread 'v2::tests::grid::received_advertisement_after_backing_leads_to_acknowledgement' panicked at polkadot/node/network/statement-distribution/src/v2/tests/grid.rs:528:13:

tl;dr: TIL that custom asserts should always be macros.

@@ -57,8 +59,7 @@ fn backed_candidate_leads_to_advertisement() {
let candidate_hash = candidate.hash();

let other_group_validators = state.group_validators(local_group_index, true);
let target_group_validators =
state.group_validators((local_group_index.0 + 1).into(), true);
let target_group_validators = state.group_validators(other_group, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This naming was already confusing to me and this change makes it a little more so. Can we take this opportunity to rename it everywhere here? Maybe just use local_group and other_group (or our_group and outside_group like you have below) and have a comment that says we're targeting the other_group because this is testing grid mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks for calling it out. Not sure about our and outside - for groups it kind of makes sense, but IMO outside_para would be a head-scratcher. To me, local and other make sense: local_validator, local_para, other_para, etc. I'll standardize on those.

Multiple people expressed confusion over the previous naming. This new scheme
just keeps things more consistent and clear.
@mrcnski mrcnski added T0-node This PR/Issue is related to the topic “node”. R0-silent Changes should not be mentioned in any release notes labels Dec 4, 2023
@mrcnski mrcnski merged commit f240e02 into master Dec 5, 2023
116 checks passed
@mrcnski mrcnski deleted the mrcnski/statement-distribution-test-incoming-ack branch December 5, 2023 11:15
ordian added a commit that referenced this pull request Dec 11, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
ordian added a commit that referenced this pull request Dec 15, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

5 participants