Closed
Bug 1313485
Opened 6 years ago
Closed 6 years ago
Convert XPCOM test TestJemalloc to a gtest
Categories
(Core :: Memory Allocator, defect)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
Details
Attachments
(1 file)
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/.
![]() |
||
Comment 1•6 years ago
|
||
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•6 years ago
|
||
> Note that this one should actually end up in memory/tests/gtest/
Or just memory/gtest/ might be better.
Assignee | ||
Updated•6 years ago
|
Component: XPCOM → Memory Allocator
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: 5yzn8o33Ne5
Attachment #8806967 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment 4•6 years ago
|
||
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•6 years ago
|
||
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+
Assignee | ||
Comment 6•6 years ago
|
||
(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)|.
Assignee | ||
Comment 7•6 years ago
|
||
(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•6 years ago
|
||
> 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()?
Assignee | ||
Comment 9•6 years ago
|
||
(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.
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f94837b76324
Assignee | ||
Comment 11•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b545e55e42aae19c6ba66f316b3b8167732be4b Bug 1313485 - Convert XPCOM test TestJemalloc to a gtest. r=njn
![]() |
||
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e88332649e54
Comment 14•6 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff2318cfe87 OS X bustage requires clobber to fix. r=clobber
Assignee | ||
Comment 15•6 years ago
|
||
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4ff2318cfe87
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 17•6 years ago
|
||
(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 → ---
Assignee | ||
Comment 18•6 years ago
|
||
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.
Assignee | ||
Comment 19•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea97ec1c574330bc76f453058f2fa8e55027db3d Bug 1313485 - Convert XPCOM test TestJemalloc to a gtest. r=njn
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ea97ec1c5743
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•