Skip to content

chore: All tests use builders#600

Merged
dvdplm merged 16 commits into
mainfrom
dvdplm/fix/testing-material-path-bug
May 22, 2026
Merged

chore: All tests use builders#600
dvdplm merged 16 commits into
mainfrom
dvdplm/fix/testing-material-path-bug

Conversation

@dvdplm
Copy link
Copy Markdown
Contributor

@dvdplm dvdplm commented May 18, 2026

What

Migrates the last set of tests to use per-test isolated temp dirs and centralized/threshold builders (centralized/threshold custodian, keygen, CRS etc).

As a consequence we can delete plenty of helpers ( threshold_handles*, centralized_custodian_handles, and the ensure_*_material_exists / setup modules in util/key_setup/test_tools.rs and testing/utils.rs).

generate-test-material now calls generate_material_to_path directly.

Gets rid of all (?) sleep(TIME_TO_SLEEP_MS) waits.

It got a lot noisier than I thought it would; apologies to reviewers but the diff is mostly mechanical. Focus review on the path/lifetime changes.

Why

The motivation for this PR is that during work on #589 I realized that there were a set of tests that are very slow in CI but fast locally (huge discrepancy: more than 200s in CI; 10s locally). It turns out that I had gotten the paths mixed up in previous PRs so many tests ended up re-generating material.

After this PR there is no longer any silent fallback to re-generate testing material at runtime; missing testing material is now a hard error.

CI has its "Generate Testing Material" job that ensures the data is there. Locally users must run:

# fast tests only
cargo run -p generate-test-material -- --profile insecure --parties 4

# add slow_tests/insecure too
cargo run -p generate-test-material -- --profile insecure,secure --parties 4,13

The above populates {WORKSPACE_ROOT}/test-material/{testing,default}/ and must be run once. Each test uses a tempdir that is created and populated by TestMaterialManager::setup_test_material_temp via a copy from the immutable pre-generated source.

Our local {WORKSPACE_ROOT}/core/service/keys folders can be safely removed now. No tests use it.

dvdplm added 4 commits May 18, 2026 13:38
… that restart servers to keep key material alive.

Delete regenerate_central_keys as it's no longer used.
…es unused helpers and unifies the call paths for

key material access.
@cla-bot cla-bot Bot added the cla-signed The CLA has been signed. label May 18, 2026
@dvdplm dvdplm marked this pull request as ready for review May 18, 2026 19:31
@dvdplm dvdplm requested a review from a team as a code owner May 18, 2026 19:31
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 18, 2026

Consolidated Tests Results 2026-05-22 - 10:14:24

Test Results

passed 7 passed

Details

tests 7 tests
clock not captured
tool junit-to-ctrf
build build-and-test arrow-right test-reporter link #2356
pull-request chore: All tests use builders link #600

test-reporter: Run #2356

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Pending ⏳ Other ❓ Flaky 🍂 Duration ⏱️
7 7 0 0 0 0 0 not captured

🎉 All tests passed!

Tests

View All Tests
Test Name Status Flaky Duration
k8s_test_crs_uniqueness 39.2s
k8s_test_insecure_keygen_encrypt_and_public_decrypt 1m 50s
k8s_test_insecure_keygen_encrypt_multiple_types 2m 6s
k8s_test_keygen_and_crs 1m 54s
k8s_test_keygen_uniqueness 4m 45s
k8s_test_centralized_insecure 54.7s
nightly_full_gen_tests_default_k8s_centralized_sequential_crs 1.6s

🍂 No flaky tests in this run.

Github Test Reporter by CTRF 💚

🔄 This comment has been updated

@dvdplm dvdplm self-assigned this May 21, 2026
Copy link
Copy Markdown
Contributor

@kc1212 kc1212 left a comment

Choose a reason for hiding this comment

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

Very nice! It's good that all these big, ugly helpers disappeared.

Here are a couple of things AI also flagged to me, they're not in the PR changes but somewhat related

  1. verify_material_exists is too shallow (material/manager.rs:95-132). It only checks the top-level dir. Consider verifying at least one expected PUB/<key_type>/<key_id> under it, or making copy_key_files return an error when an expected source is missing instead of silently skipping. Otherwise the "hard error" guarantee is leaky.
  2. generation_hint message is missing cargo run -p prefix (material/manager.rs:113-114). Says generate-test-material --profile insecure --parties 4, but the PR description and
    helpers.rs:40 use cargo run -p generate-test-material -- …. Minor docs nit.
  3. prss_from_storage_test at engine/threshold/service/epoch_manager.rs:1668 still has an undocumented sleep(Duration::from_secs(1)). Wasn't touched here, but flagging since the PR touches this surface.
  4. Missing test-material is still not a hard error.
    helpers.rs (line 22) only resolves a workspace root if test-material/ already exists; otherwise it returns a TestMaterialManager with source_path = None. Then manager.rs (line 98) explicitly skips verification when source_path is None. That means a local run without generated material does not fail at setup with the promised actionable error; it can proceed into empty tempdirs and fail later or regenerate some material.
  5. Partial or wrong-party-count fixtures are silently accepted.
    manager.rs (line 111) only checks that the profile directory exists. The actual copy helpers return Ok(()) when required files or epoch directories are missing: manager.rs (line 467), manager.rs (line 497). So test-material/testing/ can exist while the requested 13-party key, CRS, PRSS, or per-party signing files are absent, and setup still succeeds. That does not satisfy “missing testing material is now a hard error.”

Comment thread core/service/src/client/tests/centralized/custodian_backup_tests.rs Outdated
Comment thread core/service/src/testing/setup/centralized.rs
Comment thread core/service/src/testing/setup/threshold.rs
Comment thread core/service/src/client/tests/centralized/custodian_backup_tests.rs
Comment thread core/service/src/client/tests/centralized/custodian_backup_tests.rs
@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 21, 2026

Here are a couple of things AI also flagged to me, they're not in the PR changes but somewhat related

These are actually some very good points here but fixing these led to a substantial diff. I think it's better to do them in a separate PR.

dvdplm added 4 commits May 21, 2026 16:19
`generate-test-material` does not produce PRSS fixtures, never did; PRSS is bootstrapped at server startup. This PR
makes missing data a hard error, surfacing this "lie".
@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 21, 2026

These are actually some very good points here but fixing these led to a substantial diff. I think it's better to do them in a separate PR.

@kc1212 This plan fell through and I had to do the fixes here instead, adding to the noise.

The addition is essentially that this PR makes missing testing material into a hard error instead of silently re-generating the missing pieces. This surfaced a wart: our fixture setup code was pretending to copy PRSS data that was never actually generated by generate-test-material. This work is in ffdb8ab

I also had to merge in main and that ended up causing plenty of conflicts in the custodian backup tests you commented on. I choose to stay much closer to the incoming code from main as I think there were good changes merged recently (today?).

I apologise for the noisy PR. I did try to keep it contained!

kc1212
kc1212 previously approved these changes May 22, 2026
Copy link
Copy Markdown
Contributor

@kc1212 kc1212 left a comment

Choose a reason for hiding this comment

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

All looks good to me!

@dvdplm dvdplm merged commit 6729200 into main May 22, 2026
58 checks passed
@dvdplm dvdplm deleted the dvdplm/fix/testing-material-path-bug branch May 22, 2026 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The CLA has been signed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants