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

Fix BABE smoke tests #845

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix BABE smoke tests #845

wants to merge 3 commits into from

Conversation

Agusrodri
Copy link
Contributor

What does it do?

Fixes two smoke tests related to BABE, that were failing within a certain period of time in which session keys for validators were changed.

This PR aims to adapt the tests for future situations and make them consistent even if there is a change in session keys.

@Agusrodri Agusrodri added B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime/client code not-breaking Does not need to be mentioned in breaking changes labels Feb 4, 2025
Copy link
Contributor

github-actions bot commented Feb 4, 2025

WASM runtime size check:

Compared to target branch

dancebox runtime: 1412 KB (no changes) ✅

flashbox runtime: 824 KB (no changes) ✅

dancelight runtime: 2172 KB (no changes) ✅

container chain template simple runtime: 1116 KB (no changes) ✅

container chain template frontier runtime: 1404 KB (no changes) ✅

Copy link
Contributor

github-actions bot commented Feb 4, 2025

Coverage Report

(master)

@@                      Coverage Diff                       @@
##           master   agustin-fix-babe-smoke-test     +/-   ##
==============================================================
  Coverage   66.04%                        66.04%   0.00%     
  Files         335                           335             
  Lines       58858                         58858             
==============================================================
  Hits        38869                         38869             
  Misses      19989                         19989             
Files Changed Coverage

Coverage generated Tue Feb 4 23:11:05 UTC 2025

`Missing babe key for block author: ${expectedAuthor}`
).toBeTruthy();
const pubKey = hexToU8a(authorKeys.babe);
const authorBabe = accountsWithBabeKeys.find((acc) => acc[0] == expectedAuthor);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think will not be covering some cases, for instance, what happens if we are adding or removing one validator?

Here the more important problem is that we are doing the following:

  • in the first test, we are going to the block where the session started, we are getting the validators that are outputed by the digest log and compare them against the current set of babe validaotrs:
    // Assert that authorities from log == authorities from pallet

This is wrong, as the digest in session N is expressing the babe validators for session N+1, and we are comparing it against those in session N

What we should do instead is go to the session N-1 start block, get the digest, and compare it against those in session N

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime/client code not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants