Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add tests to ensure session is set correctly in OnChainScrapingVotes #4821

Merged
merged 6 commits into from
Feb 2, 2022

Conversation

Lldenaurois
Copy link
Contributor

@Lldenaurois Lldenaurois commented Jan 31, 2022

Add assertions to runtime tests in order to ensure that session in OnChainScrapingVotes matches the expected session.

Addition to #4810

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 31, 2022
@Lldenaurois Lldenaurois changed the title Add tests to ensure session is set correctly Add tests to ensure session is set correctly in OnChainScrapingVotes Jan 31, 2022
@Lldenaurois Lldenaurois added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Jan 31, 2022
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A indirect test, but it covers it

@ordian
Copy link
Member

ordian commented Feb 1, 2022

I don't think it covers it. I've checked it before, it passed on master before #4810, because it triggers only else branch here

if let Some(ref mut value) = value {
value.backing_validators_per_candidate.clear();
value.backing_validators_per_candidate.extend(backing_validators_per_candidate);
} else {
*value = Some(ScrapedOnChainVotes::<T::Hash> {
backing_validators_per_candidate,
disputes: MultiDisputeStatementSet::default(),
session,
});
}

@drahnr drahnr self-requested a review February 1, 2022 10:44
@drahnr
Copy link
Contributor

drahnr commented Feb 1, 2022

Just stepped through with a debugger, @ordian is correct. I am working on a separate test case atm

@drahnr drahnr force-pushed the scraping_on_chain_votes_test branch from d76a5bd to 3034ff6 Compare February 2, 2022 09:41
@drahnr drahnr force-pushed the scraping_on_chain_votes_test branch from 8925da0 to 86ff7fd Compare February 2, 2022 11:57
@drahnr drahnr mentioned this pull request Feb 2, 2022
2 tasks
@drahnr drahnr self-assigned this Feb 2, 2022
@@ -1313,6 +1313,10 @@ fn check_signature(
}
}

#[cfg(test)]
#[allow(unused_imports)]
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 it needed? isn't it used when cfg(test) is enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw an error in CI about unused imports, hence I added it. This will be refactored soon after #4834 since there is quite a bit of duplicate test code.

@drahnr drahnr merged commit 498a797 into master Feb 2, 2022
@drahnr drahnr deleted the scraping_on_chain_votes_test branch February 2, 2022 17:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants