Closed Bug 1124058 Opened 5 years ago Closed 5 years ago

Move TestSynchronization.cpp to gtest and enable it; r=froydnj

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file)

No description provided.
Comment on attachment 8552229 [details] [diff] [review]
Move TestSynchronization.cpp to gtest and enable it

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

::: xpcom/tests/gtest/TestSynchronization.cpp
@@ +7,3 @@
>  #include "mozilla/CondVar.h"
>  #include "mozilla/Monitor.h"
> +#include "mozilla/ReentrantMonitor.h"

I don't understand why you need this extra #include and you are changing all the Monitors to ReentrantMonitors.  AFAICS, you're never acquiring the monitor(s) on the same thread re-entrantly, so Monitor should suffice.

@@ +289,2 @@
>  {
> +    ReentrantMonitor m1("automonitor");

Now Monitor isn't being tested at all. :(
Attachment #8552229 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> Comment on attachment 8552229 [details] [diff] [review]
> Move TestSynchronization.cpp to gtest and enable it
> 
> Review of attachment 8552229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/tests/gtest/TestSynchronization.cpp
> @@ +7,3 @@
> >  #include "mozilla/CondVar.h"
> >  #include "mozilla/Monitor.h"
> > +#include "mozilla/ReentrantMonitor.h"
> 
> I don't understand why you need this extra #include and you are changing all
> the Monitors to ReentrantMonitors.  AFAICS, you're never acquiring the
> monitor(s) on the same thread re-entrantly, so Monitor should suffice.

Nope, I'm acquiring them reentrantly all the time, in fact!  See the end of the TEST(Synchronization, Sanity) function for just one very simple example.

> @@ +289,2 @@
> >  {
> > +    ReentrantMonitor m1("automonitor");
> 
> Now Monitor isn't being tested at all. :(

I think this test was written back when Monitor was a reentrant monitor, and has just been disabled since that changed.  The changes from Monitor to ReentrantMonitor were done only in places where we are testing the reentrant behavior.  I had to make those changes so that the test passes.

The MonitorContention test still examines Monitors but it may not be obvious from the diff.
Flags: needinfo?(nfroyd)
(I think applying this patch and looking at the final cpp file may make it easier to review.)
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #3)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> > @@ +289,2 @@
> > >  {
> > > +    ReentrantMonitor m1("automonitor");
> > 
> > Now Monitor isn't being tested at all. :(
> 
> I think this test was written back when Monitor was a reentrant monitor, and
> has just been disabled since that changed.  The changes from Monitor to
> ReentrantMonitor were done only in places where we are testing the reentrant
> behavior.  I had to make those changes so that the test passes.

I did not realize that Monitor used to work that way!  Well, +1 for separate names for separate functionality, but...bleh.

OK, I must have not been looking at the right hunk of the diff when I was complaining here.  My mistake!
Flags: needinfo?(nfroyd)
Attachment #8552229 - Flags: feedback+ → review+
No worries, it was more natural to me because I remembered bug 556214.  :-)
https://hg.mozilla.org/mozilla-central/rev/60b46886054a
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.