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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[(no longer active)]

Attachment: Move TestSynchronization.cpp to gtest and enable it

Comment 2

[Nathan Froyd [:froydnj]]

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. :(

Comment 3

[(no longer active)]

(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.

Comment 4

[(no longer active)]

(I think applying this patch and looking at the final cpp file may make it easier to review.)

Comment 5

[Nathan Froyd [:froydnj]]

(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!

Comment 6

[(no longer active)]

No worries, it was more natural to me because I remembered bug 556214.  :-)

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60b46886054a