Closed Bug 1286161 Opened 4 years ago Closed 4 years ago

Add a GTest suite for SourceBuffer to ImageLib

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files, 2 obsolete files)

SourceBuffer is of fundamental importance in ImageLib and it really needs a GTest suite. Now is a good time to do it since I'm adding a couple of small features. (The bugs will block this one.)
Depends on: 1286165
Depends on: 1286170
Depends on: 1286175
This patch just exposes some constants which are useful for the test suite in
part 2.
Attachment #8770046 - Flags: review?(edwin)
This patch adds a fairly comprehensive test suite for SourceBuffer. It includes
coverage for the support for null IResumables and advancing by specific amounts
that have been added in recent patches.

Apologies for how much code is here; a new GTest suite is always a bear to
review. =)
Attachment #8770048 - Flags: review?(edwin)
Comment on attachment 8770048 [details] [diff] [review]
(Part 2) - Add a GTest suite for SourceBuffer.

Review of attachment 8770048 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/gtest/TestSourceBuffer.cpp
@@ +89,5 @@
> +  {
> +    EXPECT_TRUE(NS_SUCCEEDED(mSourceBuffer->Append(aData, aLength)));
> +  }
> +
> +  void CheckedAppendToBufferLastByeForLength(size_t aLength)

nit: "Bye"

@@ +627,5 @@
> +    CheckedAdvanceIteratorStateOnly(iterator, 1, 2, totalLength,
> +                                    AdvanceMode::eAdvanceByLengthExactly);
> +    CheckData(iterator.Data(), offset++, iterator.Length());
> +  }
> +  

nit: whitespace

@@ +680,5 @@
> +
> +  // Advance past the new data and wait again.
> +  CheckedAdvanceIterator(iterator, firstWriteLength);
> +  CheckIteratorMustWait(iterator, mCountResumes);
> +  

nit: whitespace
Attachment #8770048 - Flags: review?(edwin) → review+
Thanks for the review! I've addressed the issues above and also added one new
test, which ensures that we don't advance to a new chunk if the iterator is at a
chunk boundary and we do a zero-byte read. (In other words, it tests the tweak I
added in the updated patch in bug 1286175.)
Attachment #8770048 - Attachment is obsolete: true
Comment on attachment 8770704 [details] [diff] [review]
(Part 2) - Add a GTest suite for SourceBuffer.

Review of attachment 8770704 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/gtest/TestSourceBuffer.cpp
@@ +632,5 @@
> +  // Make sure we reached the end.
> +  CheckIteratorIsComplete(iterator, 2, totalLength);
> +}
> +
> +TEST_F(ImageSourceBuffer, SubchunkZeroByteAdvance)

Actually, it's two new tests. This one...

@@ +687,5 @@
> +  // Make sure we reached the end.
> +  CheckIteratorIsComplete(iterator, 2, totalLength);
> +}
> +
> +TEST_F(ImageSourceBuffer, SubchunkZeroByteAdvanceWithNoData)

... and this one.

They're both very straightforward variants of existing tests.
Added one more quick test that checks that appending to a SourceBuffer after
calling Complete() has no effect.
Attachment #8770704 - Attachment is obsolete: true
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d457ff02be
(Part 1) - Expose SourceBuffer and SurfaceCache constants which are useful for testing. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/762323d53afa
(Part 2) - Add a GTest suite for SourceBuffer. r=edwin
https://hg.mozilla.org/mozilla-central/rev/a6d457ff02be
https://hg.mozilla.org/mozilla-central/rev/762323d53afa
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Duplicate of this bug: 1284033
You need to log in before you can comment on or make changes to this bug.