Skip to content
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

Add QtImageReader(QImage) constructor, setters, and a convenience header for unit tests #795

Closed
wants to merge 5 commits into from

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jan 14, 2022

Stemming from conversations over at #791, this PR adds a few things to make writing unit tests for libopenshot more convenient/developer-friendly:

  1. A new constructor overload QtImageReader(const QImage&, bool inspect=true), which allows QtImageReader to be supplied directly with a QImage to use in creating frames, instead of loading a file from disk.
    This is not directly useful to OpenShot (since it isn't accessible from the Python code via the class's JSON interface), but provides a far simpler way to construct unit test data than the DummyReader. (Which has to be supplied with a CacheBase object and filled with Frame objects for individual output frames; QtImageReader needs only an image, and will generate as many frames as necessary.)
  2. A new header file, tests/test_utils.h, which can be included in any unit test file. It contains stream output operator overloads for (so far) QColor and QSize, allowing those types to be used directly in Catch2 CHECK() calls without sacrificing useful reporting. (See below for more info.) Other types can be added as needs arise.
  3. Expanded unit testing for QtImageReader covers all of the new code, plus more of what previously lacked test covered.

QtImageReader

Non-shared-pointer QImage

As I noted, the QtImageReader constructor takes a bare QImage (by const reference), not a std::shared_ptr<QImage>. The reason for this is simple: QImage is one of several Qt types that are implicitly shared by design, and our own std::shared_ptr wrapping of the class is a wholly-unnecessary leftover from way back when it was used to replace the previous frame image datatype, Magick::Image. (Which does need to be wrapped in a smart pointer to avoid memory leaks.)

Back in 2015 when the change was made (b612f33), std::shared_ptr<Magick::Image> became std::shared_ptr<QImage> (actually, at the time it was tr1::shared_ptr)... then inertia set in, leaving it that way. And so it remains, over 7 years later.

Ideally, we would un-wrap all of our QImage objects at some point, but that will require effort and careful testing. The opportunity was available to not continue the same error when passing an image to QtImageReader(), though, so I took it. (As things currently stand, QtImageReader takes that QImage reference and wraps it in a std::shared_ptr anyway, since that's still how its member storage is still defined.)

Other methods

I also added methods to QtImageReader to change its stored image, by supplying either a new path string to load from disk, or a new QImage to generate frames with: QtImageReader::SetPath(const std::string& new_path) and QtImageReader::SetQImage(const QImage& new_image).

Again, this is intended primarily for unit testing convenience, not for OpenShot use. (It could already update the path via JSON, and still can just as before.) But the new methods potentially make it easier to set up a Timeline with multiple Clips, then vary the frames produced by the Reader(s) in order to test different aspects.

(Granted, too much of that would make for poorly-compartmentalized testing, though potentially that timeline could be set up as a test fixture for use by multiple test cases.)

Stream output operators

The advantage to defining stream output operators for types being evaluated by Catch2 is that it makes the output more readable when tests fail. As I noted in #791 (comment) while a Catch2 comparison between two QColors without the stream operator looks like this:

tests/ChromaKey.cpp:69: FAILED:
  CHECK( pix_e == expected )
with expansion:
  {?} == {?}

with the stream operator, it looks like this:

tests/ChromaKey.cpp:69: FAILED:
  CHECK( pix_e == expected )
with expansion:
  QColor(0, 204, 0, 255)
  ==
  QColor(0, 192, 0, 255)

that's very readable, compared to CheckPixel which looks like this:

tests/FFmpegReader.cpp:95: FAILED:
  CHECK( f->CheckPixel(10, 112, 21, 191, 84, 255, 5) == true )
with expansion:
  false == true

Of course, the stream operator alone doesn't make it possible to test QColor or any other class with fuzzy values... for that, we'd need to add a Catch2 Matcher. (But it is possible to do that as well, is the real point.) And stream operators can be added for any other classes that support useful comparison via arithmetic operations like ==, !=, <, >, etc.,

Testing complex types

It turns out, as a general rule, that it's not only quicker to test complex types directly inside CHECK() calls, rather than breaking them down and comparing individual values one at a time "by hand", but it's also better to do it that way. As long as the operands can be represented as strings in the Catch2 output, the failure reporting is far more robust with more complex types. (See above.)

If I'd written the test as four CHECK(color1.red() == color2.red()), CHECK(color1.green() == color2.green()), etc. calls, then when the colors don't match either the output is four times as verbose, and therefore harder to read, or only the non-matching channels are shown without the larger context of the entire color value. Either way, it's less useful information when trying to track down the problem.

cc: @nilp0inter for #791

To add flexibility, a new constructor is added that accepts a
QImage (by const reference — NOTE: NOT a std::shared_ptr),
and stores that image as its source instead of loadin from
disk.

This isn't directly useful to OpenShot (it can't be configured
via the reader's JSON interface), but came up in a discussion
on unit testing as a simpler alternative to the DummyReader.
It may also be useful to other C++ consumers of libopenshot.
@ferdnyc ferdnyc added code Source code cleanup, streamlining, or style tweaks tests Changes related to the unit tests and/or code coverage labels Jan 14, 2022
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 14, 2022

As long as the operands can be represented as strings in the Catch2 output, the failure reporting is far more robust with more complex types. (See above.)

(In fact, even better is to include the call to retrieve data directly in the CHECK() call, instead of storing it and comparing variables. Compare all of the above to this test:

tests/QtImageReader.cpp:127: FAILED:
  CHECK( i->pixelColor(10, 10) == QColor(Qt::green) )
with expansion:
  QColor(255, 0, 0, 255)
  ==
  QColor(0, 255, 0, 255)

The more information that's presented to the macro call directly, the more will be made visible on failure, and the easier it is to start tracking down the problem.

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #795 (dae4b56) into develop (3c8dc71) will increase coverage by 0.32%.
The diff coverage is 98.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #795      +/-   ##
===========================================
+ Coverage    48.97%   49.30%   +0.32%     
===========================================
  Files          184      184              
  Lines        15702    15790      +88     
===========================================
+ Hits          7690     7785      +95     
+ Misses        8012     8005       -7     
Impacted Files Coverage Δ
tests/ChromaKey.cpp 100.00% <ø> (+15.38%) ⬆️
src/QtImageReader.cpp 77.65% <97.43%> (+5.28%) ⬆️
src/QtImageReader.h 100.00% <100.00%> (+33.33%) ⬆️
tests/QtImageReader.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c8dc71...dae4b56. Read the comment docs.

@github-actions
Copy link

Merge conflicts have been detected on this PR, please resolve.

@github-actions github-actions bot added the conflicts A PR with unresolved merge conflicts label Jun 18, 2022
@ferdnyc ferdnyc closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code cleanup, streamlining, or style tweaks conflicts A PR with unresolved merge conflicts tests Changes related to the unit tests and/or code coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant