WIP: A few more Frame tests

[musteresel]

I've expanded the test suite for the Frame class a bit, and also corrected some (seemingly?) outdated comments in Frame.h. The tests still do not cover most of what a Frame can do. A lot of the behavior which I'd like to test more is actually quite ... surprising, though. Issue about that: #447

Depending on the conclusion of the issue I'll further extend these tests to capture either the existing behavior as good as possible or the IMO more consistent behavior. Therefore I mark this as "Work In Process" for now :)

[musteresel]

Regarding the failing CI: This is due to me using REQUIRE from unittest++ (since version 1.6.0) but unittest++ on these CI instances is at version 1.4 (from 2008 ..). Thoughts on that?

[SuslikV]

What is the issue with the existing tests? There is no clear explanation given.

[musteresel]

The existing tests cover almost none of Frames code; which is why I'd like to extend the tests.

[ferdnyc]

The existing tests cover almost none of Frames code; which is why I'd like to extend the tests.

Our tests desperately need more coverage, so I'm very much in favor of that under any circumstances.

As far as the Ubuntu 16.04 failures, unfortunately that release is still supported for another year+ (not 2 months, as I'd previously said -- I was off by a year), so our tests need to be compatible. If there's a way to use REQUIRE but stay compatible with 16.04, then that's fine. Maybe something along these lines?

#ifndef REQUIRE
  #define REQUIRE
#endif

(Edit: Corrected to simplify even further.)

Unfortunately we won't get an updated coverage report until the CI run passes all jobs.

[ferdnyc]

As far as the Ubuntu 16.04 failures, unfortunately that release is still supported for another year+ (not 2 months, as I'd previously said -- I was off by a year), so our tests need to be compatible. If there's a way to use REQUIRE but stay compatible with 16.04, then that's fine. Maybe something along these lines?

#ifndef REQUIRE
  #define REQUIRE
#endif

I just pushed a commit to do exactly that, we'll see if Travis likes it...

[ferdnyc]

That worked, just waiting on the rest of the builds to finish so we can get updated coverage stats.

[codecov-io]

Codecov Report

Merging #446 into develop will decrease coverage by 6.95%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #446      +/-   ##
===========================================
- Coverage    49.45%   42.50%   -6.96%     
===========================================
  Files          129      129              
  Lines        10261    13299    +3038     
===========================================
+ Hits          5075     5653     +578     
- Misses        5186     7646    +2460     

Impacted Files	Coverage Δ
include/Frame.h	100.00% <ø> (ø)
tests/Frame_Tests.cpp	100.00% <100.00%> (ø)
tests/ReaderBase_Tests.cpp	52.38% <0.00%> (-47.62%)	⬇️
src/DummyReader.cpp	49.18% <0.00%> (-27.75%)	⬇️
src/QtImageReader.cpp	52.63% <0.00%> (-13.11%)	⬇️
src/ImageReader.cpp	52.11% <0.00%> (-11.83%)	⬇️
tests/Clip_Tests.cpp	89.56% <0.00%> (-10.44%)	⬇️
src/effects/Negate.cpp	25.00% <0.00%> (-6.25%)	⬇️
src/Color.cpp	38.98% <0.00%> (-6.12%)	⬇️
src/Point.cpp	42.42% <0.00%> (-5.86%)	⬇️
... and 150 more

---

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 07a447c...d073d8c. Read the comment docs.

[musteresel]

Fixed the 44.2 to 44.1 :)

[ferdnyc]

Looking at the full coverage map for Frame.cpp as of this PR, there's a lot of stuff in there that isn't really a priority for unit-testing. (Things like Frame::Play() and Frame::Display() that really aren't API calls at all — the ones where it's open for debate whether they should even be methods of the Frame class at all.) So, 100% coverage wouldn't, IMHO, really be the goal.

But there is still plenty of other stuff we could be testing, like (just walking down the source from top to bottom...):

• Frame::operator=() assignment
• Frame::GetWaveform()
• Frame::ClearWaveform()
• Frame::GetAudioSample()
  • with channel == 0
  • and with channel > 0
• Frame::GetPlanarAudioSamples()
  • with and
  • without resampling
• Frame::GetAudioSampleBuffer()
• calling Frame::GetPixels() on a frame with no image
• calling Frame::CheckPixel() with invalid row/col
• Frame::GetSamplesPerFrame()
• calling Frame::Save() with a modified aspect ratio
• Frame::Thumbnail()
  • with and
  • without a modified aspect ratio
  • with and
  • without ignore_aspect
  • with and
  • without an overlay_path
  • with and
  • without a mask_path
• Frame::constrain()
• calling Frame::AddImage() on an interlaced frame, with non-blank source and destination images — code that I contend is currently very broken (Frame: Fix interlaced AddImage #418)
• Frame::ApplyGainRamp()
• calling Frame::GetMagickImage() on a frame with no image

(...I'm just throwing that list out there as a sort of general status-check. It's certainly not anyone's to-do list. Or, maybe it's everyone's to-do list. Meaning, the list is there — and please feel free to amend or correct it — for anyone who'd like to contribute tests for any of those branches. I'm hoping it'll help me guilt myself into writing at least a few of them, too.)

[github-actions]

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