Closed Bug 1228029 Opened 4 years ago Closed 4 years ago

Fix gtest assertions in TestJobScheduler


(Core :: Graphics, defect)

Not set



Tracking Status
firefox45 --- fixed


(Reporter: nical, Assigned: nical)



(1 file)

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.
Attached patch fix the test.Splinter Review
Attachment #8692062 - Flags: review?(bugmail.mozilla)
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.
Attachment #8692062 - Flags: review?(bugmail.mozilla) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.