This repository was archived by the owner on Feb 6, 2026. It is now read-only.
Repository navigation
chore: improve dal integration test performance - #8332
Merged
Merged
Conversation
Contributor
Author
|
/try |
Dependency Review✅ No vulnerabilities or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Okay, starting a try! I'll update this comment once it's running... |
zacharyhamm
force-pushed
the
test-improvements
branch
from
January 22, 2026 21:21
51b7fa1 to
791eedf
Compare
Contributor
Author
|
/try |
|
Okay, starting a try! I'll update this comment once it's running... |
Contributor
Author
|
/try |
|
Okay, starting a try! I'll update this comment once it's running... |
zacharyhamm
force-pushed
the
test-improvements
branch
from
January 22, 2026 22:49
7b1d46b to
58cda4a
Compare
Contributor
Author
|
/try |
|
Okay, starting a try! I'll update this comment once it's running... |
added 2 commits
January 22, 2026 16:51
Adds arguments to the dal test macro that enable and disable specific "servers" per test. By default, only the rebaser and pinga will be started, and you can enable the rest with: enable_veritech enable_edda enable_forklift Note that I have not enabled edda for any test, and all tests pass, so it looks like we don't have any real integration testing for edda at the moment. The one you'll want the most is enable_veritech, which you should be sure to enable any time you will have to execute functions. This took about 10-30 seconds off my test runs.
In read_wait_for_memory we spin checking if the requested object has landed in the memory cache (via the events stream) and then we look at disk and durable storage. This adds a 2+ second overhead for many fetches, which either are not about to land in the memory cache because they are for old objects which have been evicted, or because we just don't propagate data fast enough across NATS for it to land. Checking the normal read path before trying to spin has a huge performance result for the integration tests, easily 100 seconds faster in CI, and will likely have a positive impact throughout the stack.
zacharyhamm
force-pushed
the
test-improvements
branch
from
January 22, 2026 22:55
58cda4a to
fd02731
Compare
zacharyhamm
changed the title
zacharyhamm
marked this pull request as ready for review
January 22, 2026 22:57
Contributor
Author
|
/try |
|
Okay, starting a try! I'll update this comment once it's running... |
nickgerace
approved these changes
Jan 22, 2026
| use pretty_assertions_sorted::assert_eq; | ||
|
|
||
| #[test] | ||
| #[test(enable_veritech)] |
Contributor
There was a problem hiding this comment.
Okay I've wanted this idea for like ~3-4 years and I am SO happy to finally see it! Yes yes yes!
Comment on lines
+48
to
+63
| // Conditionally start servers based on macro arguments | ||
| if args.should_start_server("forklift") { | ||
| expander.setup_start_forklift_server(); | ||
| } | ||
| if args.should_start_server("veritech") { | ||
| expander.setup_start_veritech_server(); | ||
| } | ||
| if args.should_start_server("pinga") { | ||
| expander.setup_start_pinga_server(); | ||
| } | ||
| if args.should_start_server("edda") { | ||
| expander.setup_start_edda_server(); | ||
| } | ||
| if args.should_start_server("rebaser") { | ||
| expander.setup_start_rebaser_server(); | ||
| } |
Contributor
There was a problem hiding this comment.
Ended up being simple! Nice.
Comment on lines
+33
to
+55
| const DEFAULT_SERVERS: &[&str] = &["rebaser", "pinga"]; | ||
|
|
||
| impl Args { | ||
| /// Check if a specific server should be disabled | ||
| pub(crate) fn should_skip_server(&self, server_name: &str) -> bool { | ||
| let skip_ident = format!("skip_{server_name}"); | ||
| self.vars.iter().any(|v| v == &skip_ident) | ||
| } | ||
|
|
||
| pub(crate) fn should_enable_server(&self, server_name: &str) -> bool { | ||
| let skip_ident = format!("enable_{server_name}"); | ||
| self.vars.iter().any(|v| v == &skip_ident) | ||
| } | ||
|
|
||
| /// Check if a specific server should be started | ||
| pub(crate) fn should_start_server(&self, server_name: &str) -> bool { | ||
| if DEFAULT_SERVERS.contains(&server_name) { | ||
| !self.should_skip_server(server_name) | ||
| } else { | ||
| self.should_enable_server(server_name) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Ah this is it. I never took the time to get deep into the idea, but I'm glad it was relatively small in the end.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Two changes here:
Adds arguments to the dal test macro that enable and disable specific
"servers" per test. By default, only the rebaser and pinga will be
started, and you can enable the rest with:
enable_veritech
enable_edda
enable_forklift
Note that I have not enabled edda for any test, and all tests pass, so
it looks like we don't have any real integration testing for edda at the
moment.
The one you'll want the most is enable_veritech, which you should be
sure to enable any time you will have to execute functions.
This took about 10-30 seconds off my test runs.
And
In read_wait_for_memory we spin checking if the requested object has
landed in the memory cache (via the events stream) and then we look at
disk and durable storage. This adds a 2+ second overhead for many
fetches, which either are not about to land in the memory cache because
they are for old objects which have been evicted, or because we just
don't propagate data fast enough across NATS for it to land.
Checking the normal read path before trying to spin has a huge
performance result for the integration tests, easily 100 seconds faster
in CI, and will likely have a positive impact throughout the stack.