1059381 - SimpleTest.requestCompleteLog does not work as expected for mochitest-browser

Status: RESOLVED FIXED

Description

[Chris Manchester (:chmanchester)]

Trying to debug bug 1041594, SimpleTest.requestCompleteLog() was found not to work as expected. I'm not sure if this is a regression from mochitest structured logging, or if this was ever supported from mochitest-browser, but it needs to work.

Comment 1

[Chris Manchester (:chmanchester)]

:akachkach, were you looking into this one? How's it going?

Comment 2

[Ahmed Kachkach [:akachkach]]

I was looking at something related (passing the logger to SimpleTest to make all test logging from there), but that doesn't seem to be easily doable.

I wonder if requestCompleteLog worked before because some of the logging was done in Tester and other mochitest-browser classes, but in any case we have to fix this.

Did you have a temporary patch for this? (I remember jmaher mentionned that he started getting complete log results with something

Comment 3

[Chris Manchester (:chmanchester)]

I think jmaher took --quiet out of the in tree config to work around this. I guess I'll look into this next week if you don't beat me to it :)

Comment 4

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

I only too --quiet out of the in tree stuff in my local patch queue

Comment 5

[Chris Manchester (:chmanchester)]

Created attachment 8483006 [details] [diff] [review]
Implement SimpleTest.requestCompleteLog for mochitest browser

There was a warning in the code about name collisions, so here's an extensive try run:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=542dd2df16e1

Comment 6

[Chris Manchester (:chmanchester)]

Comment on attachment 8483006 [details] [diff] [review]
Implement SimpleTest.requestCompleteLog for mochitest browser

Ok, try is green.

Comment 7

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

Comment on attachment 8483006 [details] [diff] [review]
Implement SimpleTest.requestCompleteLog for mochitest browser

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

r=me with one nit addressed and one question answered.

::: testing/mochitest/browser-test.js
@@ +25,5 @@
>    "resource:///modules/ContentSearch.jsm");
>  
>  const SIMPLETEST_OVERRIDES =
> +  ["ok", "is", "isnot", "ise", "todo", "todo_is", "todo_isnot", "info", "expectAssertions",
> +   "requestCompleteLog"];

there is no need for a newline here, lets just keep it as two lines.

@@ +915,5 @@
> +    self.registerCleanupFunction(function() {
> +      self.__tester.dumper.structuredLogger.activateBuffering();
> +    })
> +  };
> +

could we not just call self.simpletest.requestCompleteLog() ?  I think this is more complete, but if we change one in the future we could overlook this.

Comment 8

[Chris Manchester (:chmanchester)]

(In reply to Joel Maher (:jmaher) from comment #7)
> Comment on attachment 8483006 [details] [diff] [review]
> Implement SimpleTest.requestCompleteLog for mochitest browser
> 
> Review of attachment 8483006 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with one nit addressed and one question answered.
> 
> ::: testing/mochitest/browser-test.js
> @@ +25,5 @@
> >    "resource:///modules/ContentSearch.jsm");
> >  
> >  const SIMPLETEST_OVERRIDES =
> > +  ["ok", "is", "isnot", "ise", "todo", "todo_is", "todo_isnot", "info", "expectAssertions",
> > +   "requestCompleteLog"];
> 
> there is no need for a newline here, lets just keep it as two lines.

Ok.

> 
> @@ +915,5 @@
> > +    self.registerCleanupFunction(function() {
> > +      self.__tester.dumper.structuredLogger.activateBuffering();
> > +    })
> > +  };
> > +
> 
> could we not just call self.simpletest.requestCompleteLog() ?  I think this
> is more complete, but if we change one in the future we could overlook this.

Structured logging (particularly the rewrite of buffering logic from JavaScript to Python) made SimpleTest's base requestCompleteLog incompatible with browser chrome tests, getting us into this mess in the first place.

Comment 9

[Chris Manchester (:chmanchester)]

Created attachment 8483529 [details] [diff] [review]
Implement SimpleTest.requestCompleteLog for mochitest browser.

Fixed nits. I'm intentionally including the change to browser_pass.js.

Comment 10

[Chris Manchester (:chmanchester)]

Comment on attachment 8483529 [details] [diff] [review]
Implement SimpleTest.requestCompleteLog for mochitest browser.

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

r=jmaher

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/869d64f8ab6c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/869d64f8ab6c