807607 - MFBT tests rely on MOZ_ASSERT for tests, so they don't test anything in non-DEBUG builds

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

See TestTypeTraits and TestBloomFilter.

Either use something else than MOZ_ASSERT (like TestCheckedInt) or at least #define DEBUG at the beginning of the files.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yeah, that's pretty sucky. We should see if we can reuse some of http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestHarness.h

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

I forgot about this bug, then was about to file it again, and Bugzilla's "you might also like" feature unearthed this.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: MOZ_RELEASE_ASSERT for MFBT tests

Is it pretty? No.
Is it what we want to keep using forever? I don't know.
But at least it's a straightforward fix, which should be appreciable after 1.5 years :-P

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8411044 [details] [diff] [review]
MOZ_RELEASE_ASSERT for MFBT tests

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

Cagefight to determine which patch lands.  :-)

::: mfbt/tests/TestTypeTraits.cpp
@@ +161,5 @@
>  
>  static void
>  StandardIsBaseOfTests()
>  {
> +  MOZ_RELEASE_ASSERT((IsBaseOf<B, D>::value) == true);

We could probably make the tests in this file into static_asserts, but it doesn't much matter if it's this way too.  Wonder why they weren't initially static asserts.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf3ec066bc7

(Made these static_assert's).

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cf3ec066bc7