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

RESOLVED FIXED in mozilla35

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

unspecified
mozilla35
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
:akachkach, were you looking into this one? How's it going?
Flags: needinfo?(ahmed.kachkach)
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
Flags: needinfo?(ahmed.kachkach)
(Assignee)

Comment 3

4 years ago
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 :)
I only too --quiet out of the in tree stuff in my local patch queue
(Assignee)

Comment 5

4 years ago
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
(Assignee)

Updated

4 years ago
Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
(Assignee)

Comment 6

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

Ok, try is green.
Attachment #8483006 - Flags: review?(jmaher)
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.
Attachment #8483006 - Flags: review?(jmaher) → review+
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

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

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

Updated

4 years ago
Attachment #8483006 - Attachment is obsolete: true
(Assignee)

Comment 10

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

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

r=jmaher
Attachment #8483529 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/869d64f8ab6c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.