chore: refine testing infrastructure and tests exec time comparison#426
Open
chore: refine testing infrastructure and tests exec time comparison#426
Conversation
Consolidated Tests Results 2026-02-25 - 15:39:50Test Results
Details
test-reporter: Run #3156
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
maksymsur
force-pushed
the
maksymsur/feat/2902/tests-stats
branch
from
47de3da to
e2e8779
Compare
maksymsur
changed the title
maksymsur
force-pushed
the
maksymsur/feat/2902/tests-stats
branch
10 times, most recently
from
3d1ecd9 to
2cab5f9
Compare
maksymsur
force-pushed
the
maksymsur/feat/2902/tests-stats
branch
from
2cab5f9 to
1f60535
Compare
maksymsur
changed the title
maksymsur
changed the title
dvdplm
reviewed
Feb 25, 2026
Contributor
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
Truly excellent documentation all around, nice.
Left some comments here and there.
| fn wait_for_ports_free(ports: &[u16], timeout: Duration) { | ||
| let deadline = Instant::now() + timeout; | ||
| for &port in ports { | ||
| while Instant::now() < deadline { |
Contributor
There was a problem hiding this comment.
Doesn't this mean that if one of the ports in ports is slow to free, then this single slow port will "consume" the deadline?
I think it's more correct to check all ports in the same iteration, sort of like so:
loop {
if ports.iter().all(|&p| port_is_bindable(DOCKER_ADDR, p) { return Ok(()) }
// Now check deadline, bail with error if reached
if Instant::now() >= deadline { … … … }
thread::sleep(sleep);
}Overkill I guess but we should be able to check the ports in parallel too yeah?
| let deadline = Instant::now() + timeout; | ||
| for &port in ports { | ||
| while Instant::now() < deadline { | ||
| if TcpStream::connect(("127.0.0.1", port)).is_err() { |
Contributor
There was a problem hiding this comment.
- Isn't it faster and more accurate to
bind()rather thanconnect? It should be roughly the same for most cases, but if there's any weirdness on the network,connectcould be slow/fail, whenbindsucceeds. Andbindis what we actually need yeah? - Are we positive that Docker binds to
127.0.0.1:PORTand not0.0.0.0::PORT? By only probing loopback, wouldn't we miss services that bind to all interfaces?
| DockerComposeCmd { root_path, mode } | ||
| } | ||
|
|
||
| fn ports_for_mode(&self) -> &'static [u16] { |
Contributor
There was a problem hiding this comment.
Maybe this function could be const?
| #[test_context(DockerComposeThresholdDefault)] | ||
| #[tokio::test] | ||
| #[serial(docker)] | ||
| #[ignore] |
Contributor
There was a problem hiding this comment.
So we run this manually only, i.e. someone must go in here and remove the #[ignore]?
Comment on lines
+1850
to
+1851
| let temp_dir = tempfile::tempdir().unwrap(); | ||
| let keys_folder = temp_dir.path(); |
| } | ||
| } | ||
|
|
||
| /// Decrypt a ciphertext file via threshold MPC, verifying the result matches the |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Decrypt a ciphertext file via threshold MPC, verifying the result matches the | |
| /// Decrypt a ciphertext file via threshold MPC, verifying that the result matches the |
Comment on lines
+265
to
+266
| /// Mark test as passed and print summary. | ||
| fn pass(self) { |
Contributor
There was a problem hiding this comment.
Maybe K8sTestContext should have a passed: bool member that defaults to false and that we can check here? Like it stands, if a sloppy developer calls ctx.pass() it is marked as passed even though it perhaps never even ran?
Comment on lines
+308
to
+310
| let key1 = ctx.insecure_keygen().await; | ||
| let key2 = ctx.insecure_keygen().await; | ||
| let key3 = ctx.insecure_keygen().await; |
Contributor
There was a problem hiding this comment.
Can't we tokio::join! on these and run them concurrently? If they do mostly I/O it might be a bit faster?
| required-features = ["testing"] | ||
|
|
||
| [[test]] | ||
| name = "kubernetes_test_centralized" |
Contributor
There was a problem hiding this comment.
Consider using k8s instead of the full kubernetes. :)
| // Use the same longer wait window as preprocessing so each client poll | ||
| // covers more computation time. | ||
| pub const DURATION_WAITING_ON_KEYGEN_RESULT_SECONDS: u64 = 300; | ||
|
|
Contributor
There was a problem hiding this comment.
Given these are pub values, why not use doc-comments (///)?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
Refined and improved existing testing infrastructure (CI + feature flags + architecture), improved parity between old VS new tests (incl. better logging for critical places), added k8s tests examples for the most critical scenarios:
k8s_test_insecure_keygen_encrypt_and_public_decryptandk8s_test_insecure_keygen_encrypt_multiple_typeskms-core-clientIntegration TestsSection 1 —
main/test-core-client-nightly(full nightly run)Centralized —
integration_test_isolatedvsintegration_testtest_centralized_custodian_backuptest_centralized_crsgen_securetest_centralized_restore_from_backuptest_centralized_insecureThreshold —
integration_test_isolatedvsintegration_testtest_threshold_custodian_backuptest_threshold_restore_from_backuptest_threshold_concurrent_crsnightly_tests_threshold_sequential_crsfull_gen_tests_default_threshold_sequential_crstest_threshold_mpc_context_inittest_threshold_resharetest_threshold_concurrent_preproc_keygennightly_tests_threshold_sequential_preproc_keygentest_threshold_insecuretest_threshold_mpc_context_switch_6Section 2 —
main/test-core-client(--features threshold_tests, threshold only)Threshold —
integration_test_isolatedvsintegration_testtest_threshold_custodian_backuptest_threshold_restore_from_backuptest_threshold_mpc_context_inittest_threshold_resharetest_threshold_concurrent_preproc_keygentest_threshold_insecuretest_threshold_mpc_context_switch_6Threshold — Docker-only tests (no isolated counterpart in this section)
test_threshold_concurrent_crstest_threshold_insecure_compressed_keygentest_threshold_compressed_preproc_keygentest_threshold_mpc_context_switchSection 3 —
main/test-core-client(--features testing, centralized only)Centralized —
integration_test_isolatedvsintegration_testtest_centralized_custodian_backuptest_centralized_crsgen_securetest_centralized_restore_from_backuptest_centralized_insecureCentralized — newly added isolated tests
test_centralized_insecure_compressed_keygenkmsCore Service Unit TestsSection 4 —
main/test-core-service (-F testing --lib)Centralized —
misc_tests_isolatedvsmisc_teststest_central_close_after_droptest_central_health_endpoint_availabilityCentralized —
restore_from_backup_tests_isolatedvsrestore_from_backup_teststest_insecure_central_autobackup_after_deletion_isolatedtest_insecure_central_dkg_backup_isolatedThreshold —
key_gen_tests_isolatedvskey_gen_teststest_insecure_dkg_isolatedtest_insecure_dkg)Threshold —
misc_tests_isolatedvsmisc_teststest_threshold_close_after_drop_isolatedtest_threshold_health_endpoint_availability_isolatedtest_threshold_shutdown_isolatedThreshold —
restore_from_backup_tests_isolatedvsrestore_from_backup_testsnightly_test_insecure_threshold_autobackup_after_deletion_isolatednightly_test_insecure_threshold_dkg_backup_isolatedtest_insecure_threshold_crs_backup_isolatedSection 5 —
main/test-core-service (-F slow_tests -F s3_tests -F insecure threshold)Threshold —
key_gen_tests_isolatedvskey_gen_teststest_insecure_dkg_isolatedtest_insecure_dkg)default_insecure_dkg_isolateddefault_insecure_dkg)verify_keygen_responsesreconstructs ClientKey + encrypt/decrypt with Default keys (~1.5 GB); non-isolated only runscheck_conformance(structural check only). Also: per-test copy of ~10 GB Default material adds I/Osecure_threshold_keygen_isolatedsecure_threshold_keygen_test)secure_threshold_keygen_crash_online_isolatedsecure_threshold_keygen_test_crash_online)secure_threshold_keygen_crash_preprocessing_isolatedsecure_threshold_keygen_test_crash_preprocessing)test_insecure_threshold_decompression_keygen_isolatedrun_decompression_test); isolated skips server startup overhead fromthreshold_handlesThreshold —
misc_tests_isolatedvsmisc_teststest_ratelimiter_isolatedtest_ratelimiter)test_threshold_close_after_drop_isolatedtest_threshold_health_endpoint_availability_isolatedtest_threshold_shutdown_isolatedThreshold —
restore_from_backup_tests_isolatedvsrestore_from_backup_teststest_insecure_threshold_crs_backup_isolatedCentralized —
misc_tests_isolatedvsmisc_teststest_central_close_after_drop_isolatedtest_central_health_endpoint_availability_isolatedtest_largecipher_isolatedCentralized —
restore_from_backup_tests_isolatedvsrestore_from_backup_teststest_insecure_central_autobackup_after_deletion_isolatedtest_insecure_central_dkg_backup_isolatedkms-core-clientKubernetes TestsSection 6 —
build-and-test/kind-testingThreshold —
kubernetes_test_threshold_isolatedvskubernetes_test_thresholdk8s_test_crs_uniquenessfull_gen_tests_k8s_default_threshld_sequential_crs32.861sk8s_test_keygen_and_crstest_k8s_threshld_insecure193.545sk8s_test_insecure_keygen_encrypt_and_public_decryptk8s_test_insecure_keygen_encrypt_multiple_typesk8s_test_keygen_uniquenessCentralized —
kubernetes_test_centralized_isolatedvskubernetes_test_centralizedfull_gen_tests_default_k8s_centralized_sequential_crsfull_gen_tests_k8s_default_centralzd_sequential_crs1.696sk8s_test_centralized_insecuretest_k8s_centralzd_insecure171.417sSummary: Isolated Speedup at a Glance
test_centralized_crsgen_secure)full_gen_tests_default_threshold_sequential_crs)Section 7 — CI Run (mixed
threshold_tests+testingfeatures, 41 tests total)This run includes the three newly added isolated tests for the first time and has both isolated and Docker timings, enabling direct side-by-side comparison.
Centralized —
integration_test_isolatedvsintegration_testtest_centralized_custodian_backuptest_centralized_crsgen_securetest_centralized_restore_from_backuptest_centralized_insecuretest_centralized_insecure_compressed_keygenThreshold —
integration_test_isolatedvsintegration_testtest_threshold_concurrent_crstest_threshold_custodian_backuptest_threshold_restore_from_backuptest_threshold_insecure_compressed_keygentest_threshold_mpc_context_switchtest_threshold_mpc_context_inittest_threshold_resharetest_threshold_concurrent_preproc_keygentest_threshold_insecuretest_threshold_mpc_context_switch_6test_threshold_compressed_preproc_keygennightly_tests_threshold_sequential_preproc_keygenCRS / Sequential —
integration_test_isolatedvsintegration_testfull_gen_tests_default_threshold_sequential_crsnightly_tests_threshold_sequential_crsNewly Added Isolated Tests — Same-Run Docker Comparison
test_threshold_insecure_compressed_keygentest_threshold_mpc_context_switchtest_threshold_compressed_preproc_keygenIssue ticket number and link
https://github.com/zama-ai/planning-blockchain/issues/901
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md