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.

The interesting part of the patch.

...and the mach bits.

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

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

These are necessary and convenient, in that order.

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.

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.

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

Mach bits, already r+'d.

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

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

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.

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