1314350 - Port TestTXMgr to gtest

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

Attachment: 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,

***
Followup fixes for TestTXMgr

Review commit: https://reviewboard.mozilla.org/r/90414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90414/

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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!

Comment 4

[Benjamin Smedberg]

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,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90414/diff/1-2/

Comment 5

[Benjamin Smedberg]

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 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1d6693a7f5b