Closed Bug 1313485 Opened 5 years ago Closed 5 years ago

Convert XPCOM test TestJemalloc to a gtest

Categories

(Core :: Memory Allocator, defect)

Product:
Core
Component:
Memory Allocator
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(1 file)

Convert XPCOM test TestJemalloc to a gtest
5.30 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review 
Convert xpcom/tests/TestJemalloc.cpp to a gtest and move to xpcom/tests/gtest/.
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.
> Note that this one should actually end up in memory/tests/gtest/

Or just memory/gtest/ might be better.
Component: XPCOM → Memory Allocator
MozReview-Commit-ID: 5yzn8o33Ne5
Attachment #8806967 - Flags: review?(n.nethercote)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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 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.
Attachment #8806967 - Flags: review?(n.nethercote) → review+
(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)|.
(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.
> 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()?
(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.
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]
Flags: needinfo?(erahm)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff2318cfe87
OS X bustage requires clobber to fix. r=clobber
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.
No longer blocks: 1313463
Depends on: 1315830
Flags: needinfo?(erahm)
https://hg.mozilla.org/mozilla-central/rev/4ff2318cfe87
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Depends on: 1316184
Blocks: 1313463
https://hg.mozilla.org/mozilla-central/rev/ea97ec1c5743
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Depends on: 1316653
You need to log in before you can comment on or make changes to this bug.