In case anyone was doubting the value of unit tests, I've found three bugs in the Clip code so far.

1: Nullability error

The first, I found while writing tests to cover more failure cases (exceptions thrown, etc.). I added a set of tests, and every time they tried to run, the test program crashed before completion. Turned out, it was impossible to null a clip's Reader using the setter:

openshot::Clip c("video.mp4");
c.Open();
c.Reader(nullptr);

The third line would crash with an unhandled exception, because the code for Reader(openshot::ReaderBase* new_reader) tried to make calls to the reader immediately after setting the member variable to the new pointer value, without first checking whether reader was valid. Fixed.

2: Logic error

The second one, I found just by reading the code on Codecov's website, because it's in the only one of four conditional paths that we don't currently have a test for:

Do you see it? The last condition should be

else if (enabled_video == -1 && reader && !reader->info.has_video)
    enabled_video = 0;

As a result, it was impossible to use auto-enabled video on a video file with no audio, or the video would auto-disable itself. Fixed.

3: Confusing order of operations

The third one isn't really a bug per se, but it cleans up a very confusing logical flow in what should be a simple function. The code for Clip::End used to read:

float Clip::End() const
{
        // if a time curve is present, use its length
        if (time.GetCount() > 1)
        {
                // Determine the FPS fo this clip
                float fps = 24.0;
                if (reader)
                        // file reader
                        fps = reader->info.fps.ToFloat();
                else
                        // Throw error if reader not initialized
                        throw ReaderClosed("No Reader has been initialized for this Clip.  Call Reader(*reader) before calling this method.");

                return float(time.GetLength()) / fps;
        }
        else
                // just use the duration (as detected by the reader)
                return end;
}

Setting aside the unnecessary else conditions that can be streamlined with early returns, the innermost if/else also has a throw in the else, which means it'll never get to the code after the else when the if condition is false. Which in turn means the condition doesn't need to be there at all, the throw serves as an early return.

All in all, after spinning everything the right way around, Clip::End is now reduced to:

// Get end position of clip (trim end of video), which can be affected by the time curve.
float Clip::End() const
{
        if (!reader) {
                // Throw error if reader not initialized
                throw ReaderClosed(
                        "No Reader has been initialized for this Clip. "
                        "Call Reader(*reader) before calling this method.");
        }

        if (time.GetCount() <= 1) {
                // if no time curve, just use the duration (as detected by the reader)
                return end;
        }

        float fps = reader->info.fps.ToFloat();
        return float(time.GetLength()) / fps;
}

(This third one actually has nothing to do with unit tests, it's just something I spotted while I was in the code. One nice thing about writing tests is that it makes you read the code, instead of write it, which is a much better way of evaluating it objectively.)

I also used early returns to unspool a bunch of other functions that looked like this:

void Clip::DoSomething() {
    if (reader)
    {
        // Tons and TONS of code
    
        // Like, more than would fit on one screen
    
        // No matter how big your screen is

    }
}

/* That should of course read: */
void Clip::DoSomething() {
    if (!reader)
        return;
    // All the other stuff
}

I'm starting to develop this weird theory that 16:9 aspect screens are causing programmers to be more cavalier about deeply nested functions, because their screens have more width than height at this point. It certainly seem like the art of the early return, and the preference for many separate, succinct functions over functions containing long chains of conditional logic, have been falling by the wayside in recent years.