Closed Bug 1314350 Opened 3 years ago Closed 3 years ago

Port TestTXMgr to gtest

Categories

(Core :: Editor, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file)

TestTXMgr is currently a standalone binary, but those will be impossible once we remove the XPCOM glue. This bug tracks porting to gtest. I have a patch that builds but is not yet tested.
No longer blocks: 1299187
Comment on attachment 8807158 [details]
Bug 1314350 - Port TestTXMgr to gtest. This removes the framework for testing object destruction order because that was already non-deterministic due to the cycle collector,

I'm not available until the end of March (forgot to turn off reviews/needinfo in time).  So I'll have to punt to Masayuki.
Attachment #8807158 - Flags: review?(ayg) → review?(masayuki)
Priority: -- → P3
Comment on attachment 8807158 [details]
Bug 1314350 - Port TestTXMgr to gtest. This removes the framework for testing object destruction order because that was already non-deterministic due to the cycle collector,

https://reviewboard.mozilla.org/r/90414/#review91442

::: editor/txmgr/tests/TestTXMgr.cpp
(Diff revision 1)
> -static int32_t sDestructorCount      = 0;
> -static int32_t *sDestructorOrderArr  = 0;

I'd be happy if you'd explain in the commit message why this test stops testing destruction.

(I guess you rewrite the variables with RefPtr and if they're leaked by some bugs, it's detected by the test framework.)

::: editor/txmgr/tests/TestTXMgr.cpp:417
(Diff revision 1)
>      if (mLevel >= mMaxLevel) {
>        // Only leaf nodes can throw errors!
>        mFlags |= mErrorFlags;
>      }
>  
>      nsresult result = SimpleTransaction::DoTransaction();

Hmm, I replaced |nsresult result| with |nsresult rv|. So, you must write this test with old tree... Be careful when you land this.

::: editor/txmgr/tests/TestTXMgr.cpp:452
(Diff revision 1)
> -      AggregateTransaction *tximpl =
> +      RefPtr<AggregateTransaction> tximpl =
>                new AggregateTransaction(mTXMgr, cLevel, i, mMaxLevel,
>                                         mNumChildrenPerNode, flags);
>  
> -      if (!tximpl) {
> +      result = mTXMgr->DoTransaction(tximpl);

I assume that now, new for this is infallible.

::: editor/txmgr/tests/TestTXMgr.cpp:544
(Diff revision 1)
>     *
>     *******************************************************************/
>  
>    nsCOMPtr<nsITransactionManager> mgr =
>      do_CreateInstance(NS_TRANSACTIONMANAGER_CONTRACTID, &result);
> -  if (NS_FAILED(result) || !mgr) {
> +  ASSERT_TRUE(NS_SUCCEEDED(result));

Although, if there is ASSERT_SUCCESS() and EXPECT_SUCCESS(), GTest must be easier to read for us.

::: editor/txmgr/tests/TestTXMgr.cpp
(Diff revision 1)
> -    return NS_ERROR_OUT_OF_MEMORY;
> +  EXPECT_EQ(u1, tximpl);
> -  }
> -
> -  tx = 0;
> -
> -  result = tximpl->QueryInterface(NS_GET_IID(nsITransaction), (void **)&tx);

Sigh... Removing a lot blocks and QI tests causes me needing to scroll for comparing actual old line and new line.

Although, MozReview should be able to scroll only left or right code in such case, you could've separate the patch for easier to review.

::: editor/txmgr/tests/TestTXMgr.cpp:895
(Diff revision 1)
> +   * Test transaction DoTransaction() error:
> +   *
> +   *******************************************************************/
>  
> -  TEST_TXMGR_IF_RELEASE(u2); // Don't hold onto any references!
> +  tximpl = factory->create(mgr, THROWS_DO_ERROR_FLAG);
>  
> -  if (NS_FAILED(result)) {
> +  u1 = u2 = r1 = r2 = 0;

nit: nullptr instead of 0.

::: editor/txmgr/tests/TestTXMgr.cpp:1235
(Diff revision 1)
> -nsresult
> +TEST(TestTXMgr, SimpleTest)
> -simple_test()

Wow! Now, both lines are synced again! So, until here, I may have taken some mistakes.

::: editor/txmgr/tests/TestTXMgr.cpp:1598
(Diff revision 1)
>      tximpl = factory->create(mgr, NONE_FLAG);
> +    ASSERT_TRUE(tximpl);

I wonder, factory->create() isn't infallible? You keep checking it only after it's called.

::: editor/txmgr/tests/TestTXMgr.cpp:1691
(Diff revision 1)
>    tximpl = factory->create(mgr, THROWS_UNDO_ERROR_FLAG);
>  

Hmm, but you don't check if tximpl is not nullptr here... What's the difference?

::: editor/txmgr/tests/TestTXMgr.cpp:1815
(Diff revision 1)
>      tximpl = factory->create(mgr, NONE_FLAG);
>  

IIRC, you checked if tximpl is nullptr when it's exactly same case. Please make sure if you check all or none of them.

Okay, perhaps, I should check new patch again.

And I wonder, you use EXPECT_* in most cases, but ASSERT_* isn't better? It seems one failure will cause following test failures. If so, EXPECT_* just noisy and developers need to look for the first failure from the log. Although, I've never seen the log of GTests, so, I may be wrong.

Anyway, thank you for your work!
Attachment #8807158 - Flags: review?(masayuki) → review-
Yes ->create is infallible because it comes from the test fixture.

The general rule I've been using is that I prefer EXPECT_* if it's possible to keep running the test without crashing. I use ASSERT_* if you really need an early return to avoid crashes. That's what the gtest docs seem to recommend.

And I updated the commit message, but we were already skipping the destructor ordering tests because CC makes the nondeterministic. I just removed all the supporting code because it was useless. Since destructor ordering isn't part of the API guarantees of the transaction manager, I figured that would be fine.
Comment on attachment 8807158 [details]
Bug 1314350 - Port TestTXMgr to gtest. This removes the framework for testing object destruction order because that was already non-deterministic due to the cycle collector,

https://reviewboard.mozilla.org/r/90414/#review93400

Thank you very much, according to the diff from the previous patch, this is fine.
Attachment #8807158 - Flags: review?(masayuki) → review+
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d6693a7f5b
Port TestTXMgr to gtest. This removes the framework for testing object destruction order because that was already non-deterministic due to the cycle collector, r=masayuki
https://hg.mozilla.org/mozilla-central/rev/a1d6693a7f5b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.