[test] failures #15631
[test] failures #15631
Conversation
danielxiangzl
added
the
CICD:build-failpoints-images
⏱️ 3h 36m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| ) | ||
| .await | ||
| .unwrap(); | ||
| panic!("test_fault_tolerance_of_leader_equivocation"); |
There was a problem hiding this comment.
This panic!() statement causes the test to fail before it can validate the leader equivocation behavior. Consider removing it to allow the test to complete and verify the fault tolerance mechanisms are working as expected.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
danielxiangzl
force-pushed
the
daniel-paper-failures
branch
from
f523873 to
2dd9b35
Compare
| async fn test( | ||
| &self, | ||
| swarm: Arc<tokio::sync::RwLock<Box<dyn Swarm>>>, | ||
| _report: &mut TestReport, | ||
| duration: Duration, | ||
| ) -> Result<()> { | ||
| let validators = { swarm.read().await.get_validator_clients_with_names() }; | ||
| // 10 vals, test 1,2,3 failures | ||
| let num_bad_leaders = 3; | ||
| for (name, validator) in validators[..num_bad_leaders].iter() { | ||
| validator | ||
| .set_failpoint( | ||
| "consensus::leader_equivocation".to_string(), | ||
| "return".to_string(), | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| anyhow!( | ||
| "set_failpoint to set consensus leader equivocation on {} failed, {:?}", | ||
| name, | ||
| e | ||
| ) | ||
| })?; | ||
| }; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The test currently sets up the failpoints but returns immediately without running for the specified test duration. Consider adding tokio::time::sleep(duration).await before returning to ensure the test runs for the full duration after the failpoints are configured. This will provide more realistic test coverage of how the system behaves under sustained equivocation conditions.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| async fn test( | ||
| &self, | ||
| swarm: Arc<tokio::sync::RwLock<Box<dyn Swarm>>>, | ||
| _report: &mut TestReport, | ||
| duration: Duration, | ||
| ) -> Result<()> { | ||
| let validators = { swarm.read().await.get_validator_clients_with_names() }; | ||
| // 10 vals, test 1,2,3 failures | ||
| let num_bad_leaders = 1; | ||
| for (name, validator) in validators[..num_bad_leaders].iter() { | ||
| validator | ||
| .set_failpoint( | ||
| "consensus::leader_equivocation".to_string(), | ||
| "return".to_string(), | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| anyhow!( | ||
| "set_failpoint to set consensus leader equivocation on {} failed, {:?}", | ||
| name, | ||
| e | ||
| ) | ||
| })?; | ||
| }; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The test currently returns immediately after setting the failpoint, without waiting for the specified duration. This means the test may complete before the system has had time to exhibit the failure behavior being tested. Consider adding tokio::time::sleep(duration).await before returning to ensure the test runs for the full duration and properly exercises the equivocation scenario.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Description
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist