agave-validator: add args tests for run (part 3) #6918
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6918 +/- ##
========================================
Coverage 83.2% 83.3%
========================================
Files 853 854 +1
Lines 377587 377823 +236
========================================
+ Hits 314488 314903 +415
+ Misses 63099 62920 -179 🚀 New features to boost your workflow:
|
| num_rocksdb_compaction_threads: rocksdb_compaction_threads, | ||
| num_rocksdb_flush_threads: rocksdb_flush_threads, | ||
| }; | ||
| let blockstore_options = run_args.blockstore_options; |
There was a problem hiding this comment.
No need for this IMO, just use it directly below:
blockstore_options: run_args.blockstore_options,
| .map_err(|err| { | ||
| Box::<dyn std::error::Error>::from(format!( | ||
| "failed to parse rocksdb_flush_threads: {err}" | ||
| )) |
There was a problem hiding this comment.
The formatting looks off here but CI is passing 🤔
There was a problem hiding this comment.
yeah, that’s just how fmt works. they consider it valid formatting haha 🙈. no need to worry about it for now. I’ve removed these code because we don't need to wrap the error
| rocks_perf_sample_interval: value_t!(matches, "rocksdb_perf_sample_interval", usize) | ||
| .map_err(|err| { | ||
| Box::<dyn std::error::Error>::from(format!( | ||
| "failed to parse rocksdb_perf_sample_interval: {err}" | ||
| )) | ||
| })?, |
There was a problem hiding this comment.
Any reason for mapping the error here ? I think you could use ? directly on value_t!(). I would need to double check what exactly CLAP includes. If we don't like what CLAP includes, then we should write a wrapper instead of doing .map_err() on everything
Same question in a couple other places in this PR
There was a problem hiding this comment.
good catch! thank you for pointing this out! 64cfe0c
| } | ||
|
|
||
| #[test] | ||
| fn verify_args_struct_by_command_run_with_rocksdb_ledger_compression() { |
There was a problem hiding this comment.
Since we have manual parsing, maybe should have a test for invalid value too ?
Problem
#4082
Summary of Changes
- impl
FromClapArgMatchesforBlockstoreOptionsand derive PartialEq for some structsin the part 2 PR, we decided to use the same struct for both user input and core logic.
- stop parsing
rocksdb_compaction_threadsandrocksdb_flush_threadsin the thread_argsthis one is a bit tricky. I noticed we have a struct that includes all threads-related args. however, if we wanna use the same struct, the thread args will go back to the component level. in this PR, I only moved the parsing logic. the args definitions and default values are still in the original file