[gzliudan]

Proposed changes

Take TestProcessQcShallSetForensicsCommittedQc back out of the -short carve-out that #2947 draws at 4s, by removing the reason it earned one: a fixed five-second time.Sleep. Replace the sleep with a 10ms poll under a five-second deadline, and assert not only that three committed QCs were recorded but that the last one points at the voted block.

Summary

This takes TestProcessQcShallSetForensicsCommittedQc back out of the -short carve-out that #2947 draws at 4s, by removing the reason it earned one.

 func TestProcessQcShallSetForensicsCommittedQc(t *testing.T) {
   t.Parallel()
-  testshort.Skip(t)
   ... three votes, the last one triggering the commit ...
-  time.Sleep(5000 * time.Millisecond)
+  const (
+      pollInterval = 10 * time.Millisecond
+      pollTimeout  = 5 * time.Second
+      wantQCs      = 3
+  )
+  forensics := engineV2.GetForensicsFaker()
+  deadline := time.Now().Add(pollTimeout)
+  var committedQCs []types.QuorumCert
+  for {
+      committedQCs = forensics.GetCommittedQCs()
+      if len(committedQCs) >= wantQCs || time.Now().After(deadline) {
+          break
+      }
+      time.Sleep(pollInterval)
+  }
+  if assert.Len(t, committedQCs, wantQCs) {
+      assert.Equal(t, currentBlock.Hash(), committedQCs[wantQCs-1].ProposedBlockInfo.Hash)
+  }
-  assert.Equal(t, 3, len(engineV2.GetForensicsFaker().GetCommittedQCs()))
 }

The five-second sleep was there for a reason: the forensics record had no lock, so polling it read the field while the committer goroutine wrote it - polling would have turned a hidden race into a reported one rather than removing either. That is why #2944 kept this sleep when it cut the package's other fixed waits, and left the fix to #2946.

#2946 put SetCommittedQCs and GetCommittedQCs under the same mutex, and the getter now returns a copy, so the read side of a poll is safe. What was a duration becomes a deadline, so a run that records the QCs returns in milliseconds and a run that does not still fails. The poll stops as soon as the three QCs land (the >= guard), and the final assertion also checks that the last recorded QC is the QC for the voted block 905, so the test keeps teeth beyond a bare count.

Evidence

• Before: go test ./consensus/tests/engine_v2_tests -run '^TestProcessQcShallSetForensicsCommittedQc$' - 5.177s, 5.160s, 5.170s.
  After: same command - 0.167s, 0.162s, 0.168s.

The test keeps its teeth. Dropping the vote that triggers the commit makes it fail on the deadline, after the same five seconds as before:

--- FAIL: TestProcessQcShallSetForensicsCommittedQc (5.20s)
     Error: "[]" should have 3 item(s), but has 0

-short on the package now skips 9 tests instead of 10 and runs 101 instead of 100 (110 top-level tests, measured on this head); the test itself passes there in 0.22s. Whole package and detector:

ok  .../consensus/tests/engine_v2_tests   65.377s        (full run)
ok  .../consensus/tests/engine_v2_tests   49.807s        (-race -short)

make generate - No stale files; make tidy - No untidy module files; make lint - 0 issues; go vet ./... - clean.

Merge Danger

Door: two-way. A test-only change; reverting restores the sleep and the gate.

Blast Radius: none outside the test. It un-gates a test that #2947 gated; #2947 has already merged into dev-upgrade.

Types of changes

• test: Adding missing tests or correcting existing tests

Impacted Components

• Consensus

Checklist

• This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
• Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
• Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
• Tested the backwards compatibility.
• Tested with XDC nodes running this version co-exist with those running the previous version.
• Relevant documentation has been updated as part of this PR
• N/A

This change only modifies a unit test: there is no devnet/testnet flow to exercise, no wire, RPC or state-format impact, and no user-facing documentation describes forensics test timing. The reverse check above (dropping the triggering vote still fails after five seconds) is the manual verification.

Related

Ref: #2944
Ref: #2946
Ref: #2947

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: dbcf5f91-d1d6-418d-aeef-1c1beb248f61

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn