1228029 - Fix gtest assertions in TestJobScheduler

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicolas Silva [:nical]]

gtest assertions aren't thread-safe on windows. they should be replaced by MOZ_ASSERTS.
Also, the ASSERT_* macros "abort" the test by returning void. That's OK when the assertion is not happening in a function call, but is very surprising otherwise.

for instance:

> void foo(int a) {
>   ASSERT_EQ(a, 42);// (1)
> }
> TEST(Foo, Bar) {
>   ASSERT_TRUE(true); // (2)
>   foo(1337);
>   foo(42);
> }

In the case of (2) that's fine, but one might expect the test to stop if the assertion (1) fails. But it doesn't since the macro just returns, so the test continues.

So I propose that we generally avoid using ASSERT_* macros in functions and only use them in the "top-level stack frame" (by lack of a better name) of the Test. Instead, we can use EXPECT* macros which won't do anything surprising.

In this bug I'll focus on fixing TestJobScheduler because I suspect the thread-safety issue is the cause of the intermittent failure on windows.

Comment 1

[Nicolas Silva [:nical]]

Attachment: fix the test.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8692062 [details] [diff] [review]
fix the test.

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

LGTM. We may want to use MOZ_RELEASE_ASSERT instead of MOZ_ASSERT if these tests are running in non-debug builds so that people developing locally in non-debug builds don't get surprised by debug-only failures on tbpl.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd566eb39b7

Comment 4

[Carsten Book [:Tomcat]]

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