-
Notifications
You must be signed in to change notification settings - Fork 58
Make src/rawtoaces_core/ be covered 100% #235
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
Make src/rawtoaces_core/ be covered 100% #235
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
==========================================
+ Coverage 85.70% 87.47% +1.76%
==========================================
Files 11 11
Lines 2197 2196 -1
Branches 331 331
==========================================
+ Hits 1883 1921 +38
+ Misses 314 275 -39
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
And rest of them in core now done too |
| assert( sumVector( out_camera_XYZ_white_point ) != 0 ); | ||
|
|
||
| return; | ||
| } |
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.
perhaps that line is unreachable because of the return, can we try removing it?
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.
yeap. that was it.
| add_executable ( | ||
| Test_DNGIdt | ||
| testDNGIdt.cpp | ||
| test_utils.cpp |
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.
Not entirely happy with exposing the rawtoaces_util code in the rawtoaces_core tests as an implicit dependency.
Hopefully we'll redo the logging soon, so the stderr capturing won't be needed anymore.
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.
hopefully I got what you mean and did what asked.
antond-weta
left a comment
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.
looks good
Signed-off-by: Aleksandr Motsjonov <[email protected]>
Signed-off-by: Aleksandr Motsjonov <[email protected]>
Signed-off-by: Aleksandr Motsjonov <[email protected]>
Signed-off-by: Aleksandr Motsjonov <[email protected]>
Signed-off-by: Aleksandr Motsjonov <[email protected]>
0f58a3a to
8fbf555
Compare
antond-weta
left a comment
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.
LGTM

Couple tests for
src/rawtoaces_core/rawtoaces_core.cpplines 706-738Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.