-
Notifications
You must be signed in to change notification settings - Fork 57
Use OIIO RAW extensions for file validation #231
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
Use OIIO RAW extensions for file validation #231
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
==========================================
+ Coverage 85.48% 85.70% +0.21%
==========================================
Files 11 11
Lines 2164 2197 +33
Branches 326 331 +5
==========================================
+ Hits 1850 1883 +33
Misses 314 314
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
You meant no breaking API changes, but API is changed - there is a new member added + whole new command line option. That is be definition API change, just minor - non-breaking one. Not sure how it all be released. That's Q to Anton.
I think we should have all new functionality added to be fully covered with unit tests. |
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.
Please cover all new functionality added with unit tests
|
Rest of it looks very good, btw! |
|
You’re right - this does introduce an API change in the strict sense, as it adds a new public member and a new command-line option. What I meant was that the change is non-breaking: existing behavior and existing calls remain unaffected. The new command-line option is informational only and simply exposes the list of RAW formats already supported internally via OIIO; it does not alter processing behavior or defaults. The tricky part is whether we want to keep the new extension check in I don’t mind adding a test to verify the reported formats if we agree this is the right direction. If the change in check_and_add_file feels too aggressive, we can either revert back to existing code or limit this logic to the GUI application instead. |
|
Looks good. |
|
Perfect, will freeze the code as-is and add the last changes. |
9ee0003 to
1b25d54
Compare
Skip hard-coding file extensions when collecting input files and instead query OIIO for the RAW extensions it actually supports. The list is initialized once and reused both for directory scanning and --list-formats. This also prepares for GUI use, where the same extension list will be needed for file dialogs and drag-and-drop filtering. No API changes. Signed-off-by: Mikael Sundell <[email protected]>
Added get_supported_formats to python bindings Signed-off-by: Mikael Sundell <[email protected]>
Signed-off-by: Mikael Sundell <[email protected]>
763219c to
d3008cf
Compare
|
First time adding tests, please input if anything is not right or missing. Tested the python binding and I can successfully request supported formats: |
| { | ||
| auto colon = group.find( ':' ); | ||
| if ( colon == std::string::npos ) | ||
| continue; |
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.
Any way we get have a unit test that will get over here and over at line 61?
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 continue statements are filters used while parsing OIIO’s extension_list, which is external input. They are internal implementation details.
The intended behavior of this function is to return the supported RAW extensions. By asserting the presence of well-known RAW formats such as .cr2 and .dng, the tests already verify that the parsing and filtering behave correctly. If any of the continue logic were incorrect, those extensions would be missing or incorrect and the test would fail.
That said, you still think an additional test is needed?
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.
Thanks for adding the tests! Ideally we should be having close to 100% coverage over all newly added code. In some cases that would require splitting some functionality into a separate function, just so it can be executed will different inputs from unit tests.
We need to get over 90% total coverage to get the OpenSSF Best Practices badge. Sasha has been working on improving the test coverage of the existing code, but ideally everybody adding new code should be doing their part as well.
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.
Just to temper expectations:
I think the three badge levels are graduated: no specific coverage requirement for "passing", 80% for silver, 90% for gold.
Every code base is different, but anecdotally, when we first started looking at improving OIIO's coverage, it was around 65% (as a sprawling, 10+ year old code base). It was easy (i.e. just a lot of hard work) to get to around 75%, since there were lots of HUGE uncovered sections that represented features we had that just weren't tested in the testsuite. But then going from 75% to 80+ was getting really hard -- it seemed like real work to eke out every additional 0.1%. We are currently at around 82% or so, and there is no low-hanging fruit left. Most of the uncovered lines now are in short isolated segments (sometimes just a few lines), and it's prohibitive to construct new tests that only add coverage to tiny portions of the code base each. While we're always striving to improve testing (and definitely aiming to get close to 100% coverage on NEW functionality), I consider 90% an essentially unattainable target for OIIO. Given the amount of work it takes to add coverage of each new block of uncovered lines, I think we could direct all development resources to test construction for literally years to get to > 90%. It would be possible, but not justifiable versus other things we could be working on.
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.
Thanks for the insight, Larry. That is great info. OIIO is indeed huge and a pretty old code base. On the other hand, rawtoaces is pretty small, and a huge chunk of the code base has been rewritten from scratch about 6 months ago. So I think we are in a good position to get good coverage. I'm not sure if 90% is attainable, but we are currently sitting at 85%, and I can't say that we had spent huge amount of time to achieve that (Sasha may want to disagree).
I'm expecting the coverage to drop somewhat when we start working on the GUI component though.
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, nah, it wasn't this hard. All branches of the code is pretty reachable. We are not at 90 or 95 only because I haven't gotten there yet. I am confident we can do 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.
Ah, very good. If you're already in high 80s and think adding more coverage isn't hard, by all means go for it. I personally believe that untested code should be presumed to be buggy. I've been humbled by my own bugs far too many times to trust myself when I don't have a test.
I'm just saying: It can be extremely hard to get close to 100%, at some point you get diminishing returns where the resources needed to write the tests are so great that they are not giving you a positive ROI versus taking a chance and fixing a bug if it is ever encountered. Don't beat yourself up about making that judgment call when you feel like you have to make it. At the end of the day, it's about using your finite development resources where they do the most good. Very often, the best use of resources is getting better test coverage. But sometimes there are bigger fish to fry. This is a matter of the project leader setting the priorities that feel right for the particular project.
|
There is |
Signed-off-by: Mikael Sundell <[email protected]>
|
@mikaelsundell I hope you can forgive me, but there was a refactoring-of-sort PR just merged that introduced some utils for tests (for setup and assertion). You might get a conflict and you might want to use those new goodies. sry about that, I was working on it for some time. |
Signed-off-by: Mikael Sundell <[email protected]>
Signed-off-by: Mikael Sundell <[email protected]>
Signed-off-by: Mikael Sundell <[email protected]>
tests/test_image_converter.cpp
Outdated
|
|
||
| const auto &exts = rta::util::supported_raw_extensions(); | ||
| OIIO_CHECK_EQUAL( exts.empty(), false ); | ||
| } |
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 sure if this test is needed.
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.
No longer needed, removed!
|
|
||
| OIIO_CHECK_EQUAL( found_cr2, true ); | ||
| OIIO_CHECK_EQUAL( found_dng, true ); | ||
| } |
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.
can we please have a negative case:
bool found_png = false;
...
if ( line == ".png" )
found_png = true;
...
OIIO_CHECK_EQUAL( found_png, false );
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.
Yes, done!
| static const std::set<std::string> extensions = [] { | ||
| std::string extensionlist; | ||
| if ( !OIIO::getattribute( "extension_list", extensionlist ) ) | ||
| return std::set<std::string>{}; |
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 believe these two lines (check) can be removed safely?
I am right that the result will be the same?
Or maybe we can assign default (empty vector) value and just call getattribute. If value is not found - it will just stay empty. no?
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.
extension_list should exist for all valid OIIO builds, now changed to:
bool ok = OIIO::getattribute( "extension_list", extensionlist );
OIIO_ASSERT( ok && "OIIO did not provide extension_list" );
…he return value Add a negative PNG case to test_parse_parameters_list_formats. Signed-off-by: Mikael Sundell <[email protected]>
Match clang-format version used in GitHub CI Signed-off-by: Mikael Sundell <[email protected]>
Skip hard-coding file extensions when collecting input files and instead query OIIO for the RAW extensions it actually supports. The list is initialized once and reused both for directory scanning and --list-formats.
This also prepares for GUI use, where the same extension list will be needed for file dialogs and drag-and-drop filtering.
No API changes, no tests added at this point.
Checklist:
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.