Skip to content

fix(tests): avoid races and leaked state in tests - #3739

Merged
mkoura merged 1 commit into
masterfrom
misc_test_races
Oct 5, 2026
Merged

mkoura merged 1 commit into
masterfrom
misc_test_races

Conversation

@mkoura

@mkoura mkoura commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator
  • test_tx_chaining: lock the whole cluster instance. The test checks how many blocks the chained Txs take, so Txs of other tests must not take the block space, and the chained Txs fill the mempool for other tests.
  • Plutus script stake address tests: respin the cluster instance when a test leaves the script stake address registered, otherwise all the following tests using the script are skipped.
  • KES negative tests: pass a copy of the pool's cold counter file to issue-op-cert, as the command would increment the shared counter if it succeeded.
  • test_rotation_pair_keeps_voting: check the epoch length in the fixture, before the respin is requested.
  • test_permanent_fork: request the respin before splitting the cluster, so it is requested even when the split fails half-way.
  • set_network_id_env: restore the original CARDANO_NODE_NETWORK_ID value instead of deleting it.

@mkoura
mkoura requested a review from saratomaz as a code owner October 5, 2026 15:39
@mkoura
mkoura requested a balanced review from Copilot October 5, 2026 15:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The KES tests still read POOL2’s shared files without declaring that pool as in use, leaving a concurrency race.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Improves E2E test isolation by preventing shared-cluster races and restoring modified state.

Changes:

  • Adds cluster/resource isolation for transaction and Plutus tests.
  • Protects shared KES counters and restores environment state.
  • Requests cluster respins earlier and performs epoch checks before mutation.
File Description
tests_plutus/​test_delegation.py Respins when script stake registration leaks.
tests_dijkstra/​test_bls_rotation.py Checks epoch duration before requesting respin.
test_rollback.py Requests respin before cluster splitting.
test_kes.py Copies cold counters before negative commands.
test_env_network_id.py Restores environment and CLI arguments via monkeypatch.
test_chain_transactions.py Locks the cluster and marks the test as heavy.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cardano_node_tests/tests/test_kes.py
- test_tx_chaining: lock the whole cluster instance. The test checks
  how many blocks the chained Txs take, so Txs of other tests must not
  take the block space, and the chained Txs fill the mempool for
  other tests.
- Plutus script stake address tests: respin the cluster instance when
  a test leaves the script stake address registered, otherwise all
  the following tests using the script are skipped.
- KES negative tests: take a pool marked as "in use" instead of a
  hardcoded one, and pass a copy of its cold counter file to
  `issue-op-cert`, as the command would increment the shared counter
  if it succeeded.
- test_rotation_pair_keeps_voting: check the epoch length in the
  fixture, before the respin is requested.
- test_permanent_fork: request the respin before splitting the
  cluster, so it is requested even when the split fails half-way.
- set_network_id_env: restore the original `CARDANO_NODE_NETWORK_ID`
  value instead of deleting it.
@mkoura
mkoura merged commit d725b06 into master Oct 5, 2026
3 checks passed
@mkoura
mkoura deleted the misc_test_races branch October 5, 2026 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants