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

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 0 - add helper function for getService magic

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.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - optionally dump about:memory and DMD after every mochitest

The interesting part of the patch.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add mach options for dumping about:memory and DMD logs

...and the mach bits.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

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;

++

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

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!

Comment 7

[Nathan Froyd [:froydnj]]

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

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 0 - add helper function for getService magic

This is the same as before, just rebased on top of shu's patch moving
things into MemoryStats.js.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 0b - add a license header and editor modelines to MemoryStats.js

These are necessary and convenient, in that order.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add dump options to runtests.py and propagate to mochitests

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.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 2 - update TestRunner.js to understand new dumping options

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.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 3 - change MemoryStats.dump to dump about:memory and DMD if instructed

This is what we really want to do: dump things!

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 4 - add mach options for dumping about:memory and DMD logs

Mach bits, already r+'d.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

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()?

Comment 17

[Nathan Froyd [:froydnj]]

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

Comment 18

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d39b1716a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ae647825db
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4c02a6bb5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/65c17a201cd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd300d079648
https://hg.mozilla.org/integration/mozilla-inbound/rev/922a45840567

I took the liberty of adding a --dump-output-directory to mach to make the testing you asked for easier.  Seemed like a trivial change; yell if you are upset or want something about it tweaked.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d8d39b1716a8
https://hg.mozilla.org/mozilla-central/rev/93ae647825db
https://hg.mozilla.org/mozilla-central/rev/cc4c02a6bb5c
https://hg.mozilla.org/mozilla-central/rev/65c17a201cd9
https://hg.mozilla.org/mozilla-central/rev/fd300d079648
https://hg.mozilla.org/mozilla-central/rev/922a45840567