Extract check_input from process_image for focused unit tests - #309
Conversation
|
|
b58c6b1 to
4bdb9d4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
=======================================
Coverage 91.89% 91.90%
=======================================
Files 17 17
Lines 3777 3781 +4
Branches 558 559 +1
=======================================
+ Hits 3471 3475 +4
Misses 306 306
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Hi @La1itchauhan, thanks for doing this! Looking good, just raised a couple minor things. |
255a167 to
bb0809c
Compare
antond-weta
left a comment
There was a problem hiding this comment.
Sorry, forgot to submit the review
| } | ||
|
|
||
| bool ImageConverter::process_image( const std::string &input_filename ) | ||
| bool ImageConverter::check_input( const std::string &input_filename ) |
There was a problem hiding this comment.
I would suggest making this a free function for now, not a method of ImageConverter. It doesn't need to be a part of the public interface. Please remove the declaration from the header. If you need the function exposed somewhere for unit testing, declare it in rawtoaces_util_priv.h instead
There was a problem hiding this comment.
Updated — check_input is now a free function declared in rawtoaces_util_priv.h (following the same pattern as fetch_missing_metadata). process_image now passes its own status and last_error_message through by reference, so its external behavior remains unchanged.
| std::string::npos, | ||
| true ); | ||
| } | ||
| void test_check_input_empty_filename() |
There was a problem hiding this comment.
That functionality has already been covered with unit tests. If you'd like to make changes to unit tests to better reflect the code layout, you may want to edit the existing tests instead of adding new ones.
There was a problem hiding this comment.
removed the duplicate test functions as you suggested to better reflect the code layout and instead updated the existing test_last_error_message_empty_filename and test_last_error_message_file_not_found tests to call check_input directly. My thinking was that this better addresses #274 by testing the validation logic independently, without introducing additional test cases. If you'd prefer those tests to remain focused on process_image and keep this change more minimal, happy to adjust.
|
Looks good! Please address the clang-format issues and I'll get this merged in. Thanks! |
Signed-off-by: La1itchauhan <chauhanlalitofficial@gmail.com>
Signed-off-by: La1itchauhan <chauhanlalitofficial@gmail.com>
Signed-off-by: La1itchauhan <chauhanlalitofficial@gmail.com>
1bbe34b to
f45f807
Compare
Signed-off-by: La1itchauhan <chauhanlalitofficial@gmail.com>
Description
This PR extracts the early input validation logic from
ImageConverter::process_image()into a dedicatedImageConverter::check_input()function.The validation logic and observable behaviour remain unchanged. This
refactor allows the validation path to be exercised directly in unit
tests without relying on the full image processing pipeline.
Closes #274.
Tests
Added direct unit tests for
check_input()covering:The existing
process_image()validation tests were retained to ensurethe public behaviour remains unchanged.
Checklist: