-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: introduce the new MockJsonHandler
and MockParquetHandler
#678
base: main
Are you sure you want to change the base?
test: introduce the new MockJsonHandler
and MockParquetHandler
#678
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #678 +/- ##
==========================================
+ Coverage 84.08% 84.14% +0.06%
==========================================
Files 77 77
Lines 17823 18027 +204
Branches 17823 18027 +204
==========================================
+ Hits 14986 15169 +183
- Misses 2120 2144 +24
+ Partials 717 714 -3 ☔ View full report in Codecov by Sentry. |
MockEngine
that returns the new MockJsonHandler
and MockParquetHandler
MockEngine
that returns the new MockJsonHandler
and MockParquetHandler
MockJsonHandler
and MockParquetHandler
/// This handler maintains a queue of expected read calls and their results, | ||
/// enforcing that calls occur in a defined order. | ||
struct MockHandler { | ||
expected_file_reads_params: Mutex<VecDeque<ExpectedFileReadParams>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Mutex
is used because theread_{json|parquet}_files
method part of the {Json|Parquet}Handler
's traits cannot take a mutable reference to self, which is required to remove elements from the expected calls queue. An alternative approach would be to use a pointer to track the position of expected_file_reads_params
, but I opted for this simpler solution.
kernel/src/log_segment/tests.rs
Outdated
} | ||
} | ||
|
||
// Initialize mock engine and handlers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is a lot of setup code required for the mock 🤔
Also while I think some testing infrastructure is in order, I don't think this is fleshed out enough to be exposed as a general testing util. For instance, I want to be able to control what data we return.
If the mock handlers are just asserting that we get the expected paths, then I think this should only be part of the tests for the iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be interested in what @zachschuermann thinks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I definitely think some of the testing setup can still be abstracted. I also think this example looks large but most of the code is for creating the expected parameters that the handlers are passed - which can be refactored into helper functions that I foresee many tests can benefit from, e.g. creating Vec<ParsedLogPath>
Also, the mock handlers do have functionality to control what data is returned.
Each expect_read_call
takes the 3 params we expect to be passed to our handlers along with result: DeltaResult<FileDataReadResultIterator>
, which is the iterator of data batches returned from the handler when called. I just pushed an update to demonstrate this in the unit test.
/// Create a ParsedLogPath from this FileMeta | ||
#[cfg(test)] | ||
fn to_parsed_log_path(&self) -> ParsedLogPath { | ||
ParsedLogPath::try_from(self.clone()).unwrap().unwrap() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be its own function.
84e6010
to
6ac530d
Compare
MockJsonHandler
and MockParquetHandler
MockJsonHandler
and MockParquetHandler
What changes are proposed in this pull request?
This PR introduces the
MockEngine
which returns the newMockJsonHandler
, andMockParquetHandler
.These mock handlers enable functional testing without relying on real implementations of the
Engine
trait that perform actual file I/O. Instead, they simulate file reads in a controlled manner, ensuring tests remain isolated and predictable.Both handlers extend a generic
MockHandler
, which:This addition aims to better our current testing utilities. Tests that leverage
LocalMockTable
are candidates for replacement asLocalMockTable
relies on performing operations in a temporary directory, introducing unnecessary dependencies on file I/O.How was this change tested?
Included in this PR is an example of the handlers usage for unit testing a core piece of functionality in kernel:
test_log_replay
is_log_batch
flag