Closed Bug 940637 Opened 7 years ago Closed 7 years ago

add support for dumping about:memory and DMD logs after every mochitest

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(6 files, 3 obsolete files)

1.99 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.14 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
5.91 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
2.75 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
5.38 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
3.14 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
No description provided.
This is just a trivial little function to separate out the
Cc/SpecialPowers.Cc logic.

Ted's normally who I would ask, but I think jmaher can review this stuff.
Attachment #8334787 - Flags: review?(jmaher)
The interesting part of the patch.
Attachment #8334789 - Flags: review?(jmaher)
...and the mach bits.
Attachment #8334790 - Flags: review?(jmaher)
Comment on attachment 8334787 [details] [diff] [review]
part 0 - add helper function for getService magic

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

looks good

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +432,5 @@
> +        } catch (e) {
> +            service = SpecialPowers.Cc[className]
> +                                   .getService(SpecialPowers.Ci[interfaceName]);
> +        }
> +        return service;

++
Attachment #8334787 - Flags: review?(jmaher) → review+
Comment on attachment 8334789 [details] [diff] [review]
part 1 - optionally dump about:memory and DMD after every mochitest

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

r- as we do not take into account android for the output directory.

::: testing/mochitest/mochitest_options.py
@@ +373,5 @@
> +        [["--dump-dmd-after-test"],
> +         { "action": "store_true",
> +           "default": False,
> +           "dest": "dumpDMDAfterTest",
> +           "help": "Produce an DMD dump after each test in the directory specified "

s/an/a/

::: testing/mochitest/runtests.py
@@ +349,5 @@
> +        self.urlOpts.append("dumpOutputDirectory=%s" % encodeURIComponent(options.dumpOutputDirectory))
> +      if options.dumpAboutMemoryAfterTest:
> +        self.urlOpts.append("dumpAboutMemoryAfterTest=true")
> +      if options.dumpDMDAfterTest:
> +        self.urlOpts.append("dumpDMDAfterTest=true")

I would put logic here to ensure dumpOutputDirectory is valid if dumpAboutMemoryAfterTest is true

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +462,5 @@
>          }
>      }
>  
> +    if (TestRunner.dumpAboutMemoryAfterTest) {
> +        var dumpfile = TestRunner.dumpOutputDirectory + "/about-memory-" + TestRunner._currentTest + ".json.gz";

Are there any concerns with spaces or invalid characters in the _currentTest value?
how do we guarantee dumpOutputDirectory is a valid place?
I assume on android this will be deviceRoot()?

@@ +471,5 @@
> +            TestRunner.log("TEST-INFO | " + TestRunner.currentTestURL + " | MEMDUMP-END");
> +        }, null);
> +    }
> +
> +    if (TestRunner.dumpDMDAfterTest && typeof(DMDReportAndDump) != undefined) {

where is DMDReportAndDump defined?

::: testing/mochitest/tests/SimpleTest/setup.js
@@ +140,5 @@
> +}
> +
> +if (params.dumpDMDAfterTest) {
> +  TestRunner.dumpDMDAfterTest = true;
> +}

these are really long parameter names!  I am glad we don't type these by hand.

are these ever set to false, what is the default for these?
Attachment #8334789 - Flags: review?(jmaher) → review-
Comment on attachment 8334790 [details] [diff] [review]
part 2 - add mach options for dumping about:memory and DMD logs

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

this looks fine!
Attachment #8334790 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #5)
> ::: testing/mochitest/runtests.py
> @@ +349,5 @@
> > +        self.urlOpts.append("dumpOutputDirectory=%s" % encodeURIComponent(options.dumpOutputDirectory))
> > +      if options.dumpAboutMemoryAfterTest:
> > +        self.urlOpts.append("dumpAboutMemoryAfterTest=true")
> > +      if options.dumpDMDAfterTest:
> > +        self.urlOpts.append("dumpDMDAfterTest=true")
> 
> I would put logic here to ensure dumpOutputDirectory is valid if
> dumpAboutMemoryAfterTest is true

Sure, makes sense.

> ::: testing/mochitest/tests/SimpleTest/TestRunner.js
> @@ +462,5 @@
> >          }
> >      }
> >  
> > +    if (TestRunner.dumpAboutMemoryAfterTest) {
> > +        var dumpfile = TestRunner.dumpOutputDirectory + "/about-memory-" + TestRunner._currentTest + ".json.gz";
> 
> Are there any concerns with spaces or invalid characters in the _currentTest
> value?

_currentTest is just a number, so there are no concerns here.

> how do we guarantee dumpOutputDirectory is a valid place?

For desktop, we can check this in runtests.py as you suggest.

> I assume on android this will be deviceRoot()?

I think we'll just have to unconditionally override dumpOutputDirectory for runtestsremote.py.  deviceRoot is probably as good a place as any to put stuff.

> @@ +471,5 @@
> > +            TestRunner.log("TEST-INFO | " + TestRunner.currentTestURL + " | MEMDUMP-END");
> > +        }, null);
> > +    }
> > +
> > +    if (TestRunner.dumpDMDAfterTest && typeof(DMDReportAndDump) != undefined) {
> 
> where is DMDReportAndDump defined?

DMDReportAndDump is defined as a global function iff DMD is enabled.

> ::: testing/mochitest/tests/SimpleTest/setup.js
> @@ +140,5 @@
> > +}
> > +
> > +if (params.dumpDMDAfterTest) {
> > +  TestRunner.dumpDMDAfterTest = true;
> > +}
> 
> these are really long parameter names!  I am glad we don't type these by
> hand.
> 
> are these ever set to false, what is the default for these?

These are initialized appropriately further up in TestRunner.js.
This is the same as before, just rebased on top of shu's patch moving
things into MemoryStats.js.
Attachment #8334787 - Attachment is obsolete: true
Attachment #8336188 - Flags: review+
These are necessary and convenient, in that order.
Attachment #8336189 - Flags: review?(jmaher)
This patch is the Python side of things.  The only funky thing here is
the way the dump output directory gets processed.  I think this is a
little better than just defaulting it to tempfile.gettempdir() in the
options directly.
Attachment #8334789 - Attachment is obsolete: true
Attachment #8336191 - Flags: review?(jmaher)
This explicitly separates out things so you can verify that things are
defaulted properly and that they will be updated properly.  We don't
need to do the same thing for browser mochitests because undefined
values in gConfig are just as good as explicitly setting them to false.
Attachment #8336192 - Flags: review?(jmaher)
This is what we really want to do: dump things!
Attachment #8336194 - Flags: review?(jmaher)
Mach bits, already r+'d.
Attachment #8334790 - Attachment is obsolete: true
Attachment #8336195 - Flags: review+
Assignee: nobody → nfroyd
Attachment #8336189 - Flags: review?(jmaher) → review+
Comment on attachment 8336191 [details] [diff] [review]
part 1 - add dump options to runtests.py and propagate to mochitests

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

this looks pretty good. Please test locally with the default values for dumpOutputDirectory and a custom value.
Attachment #8336191 - Flags: review?(jmaher) → review+
Comment on attachment 8336192 [details] [diff] [review]
part 2 - update TestRunner.js to understand new dumping options

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

thanks.
Attachment #8336192 - Flags: review?(jmaher) → review+
Comment on attachment 8336194 [details] [diff] [review]
part 3 - change MemoryStats.dump to dump about:memory and DMD if instructed

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

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +416,5 @@
> +    MemoryStats.dump(TestRunner.log, TestRunner._currentTest,
> +                     TestRunner.currentTestURL,
> +                     TestRunner.dumpOutputDirectory,
> +                     TestRunner.dumpAboutMemoryAfterTest,
> +                     TestRunner.dumpDMDAfterTest);

is TestRunner.log a valid function to pass in as the first parameter to dump()?
Attachment #8336194 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #16)
> Comment on attachment 8336194 [details] [diff] [review]
> part 3 - change MemoryStats.dump to dump about:memory and DMD if instructed
> 
> Review of attachment 8336194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mochitest/tests/SimpleTest/TestRunner.js
> @@ +416,5 @@
> > +    MemoryStats.dump(TestRunner.log, TestRunner._currentTest,
> > +                     TestRunner.currentTestURL,
> > +                     TestRunner.dumpOutputDirectory,
> > +                     TestRunner.dumpAboutMemoryAfterTest,
> > +                     TestRunner.dumpDMDAfterTest);
> 
> is TestRunner.log a valid function to pass in as the first parameter to
> dump()?

Yup!  It's what we do now; I think it's because TestRunner is essentially being used as a namespace object, and not as a prototype for other objects.
Duplicate of this bug: 939914
You need to log in before you can comment on or make changes to this bug.