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

WIP: A few more Frame tests #446

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

musteresel
Copy link
Contributor

@musteresel musteresel commented Feb 22, 2020

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 :)

 - Some tests are commented out; behavior is strange / unexpected /
   inconsistent at times; this probably needs to be fixed, thus do not
   capture it in test for now.
 - Strange / unexpected / probably wrong behavior; this tests needs to
   be expanded once there's a clear API
 - This test needs to be expanded to test how (if) this affects
   underlying audio data.
@musteresel
Copy link
Contributor Author

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
Copy link
Contributor

SuslikV commented Feb 22, 2020

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

@musteresel
Copy link
Contributor Author

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

include/Frame.h Outdated Show resolved Hide resolved
@ferdnyc
Copy link
Contributor

ferdnyc commented Feb 22, 2020

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
Copy link
Contributor

ferdnyc commented Feb 22, 2020

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
Copy link
Contributor

ferdnyc commented Feb 22, 2020

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

@codecov-io
Copy link

codecov-io commented Feb 22, 2020

Codecov Report

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

Impacted file tree graph

@@             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
Copy link
Contributor Author

Fixed the 44.2 to 44.1 :)

@ferdnyc
Copy link
Contributor

ferdnyc commented Feb 23, 2020

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 github-actions bot added the conflicts A PR with unresolved merge conflicts label Jan 28, 2021
@github-actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts A PR with unresolved merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants