1313485 - Convert XPCOM test TestJemalloc to a gtest

Status: RESOLVED FIXED

Description

[Eric Rahm [:erahm]]

Convert xpcom/tests/TestJemalloc.cpp to a gtest and move to xpcom/tests/gtest/.

Comment 1

[Nicholas Nethercote [:njn]]

Note that this one should actually end up in memory/tests/gtest/ -- see the comment at the top of the file. It also doesn't need ScopedXPCOM, and TestHarness.h is only #included for `fail`, which will go away when you make it a gtest anyway.

Comment 2

[Nicholas Nethercote [:njn]]

> Note that this one should actually end up in memory/tests/gtest/

Or just memory/gtest/ might be better.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Convert XPCOM test TestJemalloc to a gtest

MozReview-Commit-ID: 5yzn8o33Ne5

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8806967 [details] [diff] [review]
Convert XPCOM test TestJemalloc to a gtest

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

::: memory/moz.build
@@ +6,5 @@
>  
>  DIRS += [
>      'mozalloc',
>      'fallible',
> +    'gtest',

I'd move this under if CONFIG['MOZ_MEMORY'] in this file, and remove the condition in gtest/moz.build

Comment 5

[Nicholas Nethercote [:njn]]

Comment on attachment 8806967 [details] [diff] [review]
Convert XPCOM test TestJemalloc to a gtest

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

::: memory/gtest/TestJemalloc.cpp
@@ +25,5 @@
>  TestThree(size_t size)
>  {
> +    ASSERT_NO_FATAL_FAILURE(TestOne(size - 1));
> +    ASSERT_NO_FATAL_FAILURE(TestOne(size));
> +    ASSERT_NO_FATAL_FAILURE(TestOne(size + 1));

Are these assertions necessary? Won't any failure in TestOne() cause failure overall?

@@ +39,5 @@
>     * various sizes beyond that.
>     */
>  
>    for (size_t n = 0; n < 16 K; n++)
> +    ASSERT_NO_FATAL_FAILURE(TestOne(n));

Ditto.

Comment 6

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Comment on attachment 8806967 [details] [diff] [review]
> Convert XPCOM test TestJemalloc to a gtest
> 
> Review of attachment 8806967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/gtest/TestJemalloc.cpp
> @@ +25,5 @@
> >  TestThree(size_t size)
> >  {
> > +    ASSERT_NO_FATAL_FAILURE(TestOne(size - 1));
> > +    ASSERT_NO_FATAL_FAILURE(TestOne(size));
> > +    ASSERT_NO_FATAL_FAILURE(TestOne(size + 1));
> 
> Are these assertions necessary? Won't any failure in TestOne() cause failure
> overall?

There's some finicky gtest behavior where if there's a failure in a sub-command (TestOne here) it doesn't propagate to the caller by default. |ASSERT_NO_FATAL_FAILURE| will check if the called function had a failure.

e.g. if |TestOne(size - 1)| has an assertion w/o the |ASSERT_NO_FATAL_FAILURE|, we'd move on to the next line, |TestOne(size)|.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8806967 [details] [diff] [review]
> Convert XPCOM test TestJemalloc to a gtest
> 
> Review of attachment 8806967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/moz.build
> @@ +6,5 @@
> >  
> >  DIRS += [
> >      'mozalloc',
> >      'fallible',
> > +    'gtest',
> 
> I'd move this under if CONFIG['MOZ_MEMORY'] in this file, and remove the
> condition in gtest/moz.build

Sure, I was thinking we might add other gtests (not gated on MOZ_MEMORY), but we can cross that bridge when we come to it.

Comment 8

[Nicholas Nethercote [:njn]]

> There's some finicky gtest behavior where if there's a failure in a
> sub-command (TestOne here) it doesn't propagate to the caller by default.
> |ASSERT_NO_FATAL_FAILURE| will check if the called function had a failure.

Would that be necessary if you used ASSERT in TestOne() instead of EXPECT()?

Comment 9

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > There's some finicky gtest behavior where if there's a failure in a
> > sub-command (TestOne here) it doesn't propagate to the caller by default.
> > |ASSERT_NO_FATAL_FAILURE| will check if the called function had a failure.
> 
> Would that be necessary if you used ASSERT in TestOne() instead of EXPECT()?

Yes.

Comment 10

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f94837b76324

Comment 11

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b545e55e42aae19c6ba66f316b3b8167732be4b
Bug 1313485 - Convert XPCOM test TestJemalloc to a gtest. r=njn

Comment 12

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Backed out for bustage:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1b545e55e42aae19c6ba66f316b3b8167732be4b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38788964&repo=mozilla-inbound

[task 2016-11-07T22:34:59.113221Z] creating ./config.data
[task 2016-11-07T22:34:59.165171Z] Creating config.status
[task 2016-11-07T22:34:59.321061Z] Reticulating splines...
[task 2016-11-07T22:34:59.416441Z] Traceback (most recent call last):
[task 2016-11-07T22:34:59.416478Z]   File "/home/worker/checkouts/gecko/js/src/../../configure.py", line 107, in <module>
[task 2016-11-07T22:34:59.416499Z]     sys.exit(main(sys.argv))
[task 2016-11-07T22:34:59.416525Z]   File "/home/worker/checkouts/gecko/js/src/../../configure.py", line 31, in main
[task 2016-11-07T22:34:59.416543Z]     return config_status(config)
[task 2016-11-07T22:34:59.416571Z]   File "/home/worker/checkouts/gecko/js/src/../../configure.py", line 102, in config_status
[task 2016-11-07T22:34:59.416597Z]     return config_status(args=[], **encode(sanitized_config, encoding))
[task 2016-11-07T22:34:59.416627Z]   File "/home/worker/checkouts/gecko/python/mozbuild/mozbuild/config_status.py", line 147, in config_status
[task 2016-11-07T22:34:59.416647Z]     definitions = list(definitions)
[task 2016-11-07T22:34:59.416676Z]   File "/home/worker/checkouts/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 193, in emit
[task 2016-11-07T22:34:59.416698Z]     objs = list(self._emit_libs_derived(contexts))
[task 2016-11-07T22:34:59.416729Z]   File "/home/worker/checkouts/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 206, in _emit_libs_derived
[task 2016-11-07T22:34:59.416750Z]     % lib.link_into, contexts[lib.objdir])
[task 2016-11-07T22:34:59.416772Z] mozbuild.frontend.reader.SandboxValidationError: 
[task 2016-11-07T22:34:59.416790Z] ==============================
[task 2016-11-07T22:34:59.416807Z] ERROR PROCESSING MOZBUILD FILE
[task 2016-11-07T22:34:59.416823Z] ==============================
[task 2016-11-07T22:34:59.416835Z] 
[task 2016-11-07T22:34:59.416861Z] The error occurred while processing the following file or one of the files it includes:
[task 2016-11-07T22:34:59.416874Z] 
[task 2016-11-07T22:34:59.416897Z]     /home/worker/checkouts/gecko/memory/gtest/moz.build
[task 2016-11-07T22:34:59.416909Z] 
[task 2016-11-07T22:34:59.416934Z] The error occurred when validating the result of the execution. The reported error is:
[task 2016-11-07T22:34:59.416946Z] 
[task 2016-11-07T22:34:59.416965Z]     FINAL_LIBRARY ("xul-gtest") does not match any LIBRARY_NAME
[task 2016-11-07T22:34:59.416978Z] 
[task 2016-11-07T22:34:59.416987Z]

Comment 13

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e88332649e54

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff2318cfe87
OS X bustage requires clobber to fix. r=clobber

Comment 15

[Eric Rahm [:erahm]]

Alright we're going to move building of memory/gtest to toolkit/toolkit.build where we know xul will be available. Also I filed bug 1315830 to just remove the offending SimpleProgram that has started to fail to build.

Comment 16

[Wes Kocher (:KWierso)]

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

Comment 17

[Eric Rahm [:erahm]]

(In reply to Wes Kocher (:KWierso) from comment #16)
> https://hg.mozilla.org/mozilla-central/rev/4ff2318cfe87

This was just the clobber from the backout landing, the test still hasn't landed.

Comment 18

[Eric Rahm [:erahm]]

I'm reasonably sure I've figured out the reason the nsIEnumerator test started failing to build. By removing the last 3 GeckoCppUnitTests we removed the gecko linkage from xpcom/tests. There's a quirk that if you have a GeckoCppUnitTest or GeckoSimplePrgoram in a build file the rest of the programs declared also get gecko linkage. So in the end SimplePrograms (not GeckoSimplePrograms) lost there xul linkage and started failing to build.

Comment 19

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea97ec1c574330bc76f453058f2fa8e55027db3d
Bug 1313485 - Convert XPCOM test TestJemalloc to a gtest. r=njn

Comment 20

[Carsten Book [:Tomcat]]

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