Closed Bug 1110956 Opened 6 years ago Closed 6 years ago

Port TestStrings.cpp to gtest and enable it

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

Attached patch Patch (v1)Splinter Review
The attached patch is a diff -w, because I also fixed the indentation in this test.
Attachment #8535769 - Flags: review?(nfroyd)
Comment on attachment 8535769 [details] [diff] [review]
Patch (v1)

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

Fabulous.

::: xpcom/glue/tests/gtest/TestStrings.cpp
@@ +479,1 @@
>    */

I wonder why this test is commented out?  It looks like it should succeed...
Attachment #8535769 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #1)
> ::: xpcom/glue/tests/gtest/TestStrings.cpp
> @@ +479,1 @@
> >    */
> 
> I wonder why this test is commented out?  It looks like it should succeed...

Who can say?!  But it does succeed, I turned it on.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d468b88bdb23
(In reply to Wes Kocher (:KWierso) from comment #4)
> Looks like this also broke gtests on ASAN builds:
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=4611580&repo=mozilla-inbound

Ah, this is why it's bad to leave tests disabled like this. :(
Flags: needinfo?(ehsan.akhgari)
Actually it was my mistake, I converted a raw pointer to an nsRefPtr while forgetting to remove Release() calls on it. :/

Benjamin, do you remember what happened the last time that we discussed disallowing direct ->AddRef/Release() calls on our smart pointers?
Flags: needinfo?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/b648cb645fb3
https://hg.mozilla.org/mozilla-central/rev/40d720f4c1f2
https://hg.mozilla.org/mozilla-central/rev/96f1307908bf
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
No I don't remember.
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.