Closed Bug 805116 Opened 7 years ago Closed 6 years ago

Print all the logcats? Print ALL the logcats on mobile

Categories

(Testing :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla19

People

(Reporter: wlach, Assigned: wlach)

References

Details

(Whiteboard: [leave open])

Attachments

(2 files, 1 obsolete file)

In a conversation with :kats, we figured it would be helpful to developers if we printed more of the logcat for mobile mochitest/reftest/talos. This is an easy change to make, so we should probably just do it. The fact that we clear the logcat before the tests start should keep the log from getting to an unreasonable size (or at least that's my theory).
+1 ... as long as it can be done "safely".

Also, it seems to me that talos doesn't display logcat when the test fails...which is inconvenient/unexpected/less-than-helpful!
(In reply to Geoff Brown [:gbrown] from comment #1)

> Also, it seems to me that talos doesn't display logcat when the test
> fails...which is inconvenient/unexpected/less-than-helpful!

Yes, we should fix that too.
Here's a patch that makes us print out all the logcat information, minus a bunch of Fennec messages which just seem to be noise (probably need a longer list).

This is not a finished patch; for one thing, we need to commit the mozdevice change to the github repo first. But I am interested in your feedback.
Attachment #674831 - Flags: feedback?(jmaher)
Attachment #674831 - Flags: feedback?(gbrown)
Comment on attachment 674831 [details] [diff] [review]
Print all the logcats (with a bit of filtering)

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

we still have talos to worry about as well.

::: build/mobile/remoteautomation.py
@@ +13,5 @@
>  from devicemanager import NetworkTools, DMError
>  
> +# signatures for logcat messages that we don't care about much
> +android_logcat_filters = [ "The character encoding of the HTML document was not declared",
> +                           "Use of Mutation Events is deprecated. Use MutationObserver instead." ]

very cool.

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +494,2 @@
>          buf = StringIO.StringIO()
>          self.shell(['/system/bin/logcat', '-c'], buf, root=True)

would we want to not do the logcat -c?  we do this before we launch fennec, so I am not really worried about missing anything.
Attachment #674831 - Flags: feedback?(jmaher) → feedback+
Comment on attachment 674831 [details] [diff] [review]
Print all the logcats (with a bit of filtering)

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

Bravo!

At the risk of adding more and more features to this bug, I'll mention that I also like timestamps, which could be added simply by adding "-v time" or "-v threadtime" to the logcat command.

::: build/mobile/remoteautomation.py
@@ +12,5 @@
>  from automation import Automation
>  from devicemanager import NetworkTools, DMError
>  
> +# signatures for logcat messages that we don't care about much
> +android_logcat_filters = [ "The character encoding of the HTML document was not declared",

It looks to me like there is a mismatch between "android_logcat_filters" here and "fennecLogcatFilters", later.
Attachment #674831 - Flags: feedback?(gbrown) → feedback+
Filed bug 805411 to take care of adding better logcat methods to mozdevice.
Depends on: 805411
Try run looking pretty good:

https://tbpl.mozilla.org/?tree=Try&rev=c91d1dc89174

There was one orange that had nothing to do with this patch, which nicely demonstrates the better logging output: https://tbpl.mozilla.org/php/getParsedLog.php?id=16454462&tree=Try&full=1
This is a pretty seperate patch from what I was working on earlier, but it would be a big win for debuggability of talos tests (as gbrown pointed out, not printing out the logcat on failure really reduces usefulness). 

It will become even more useful if/when we expand the amount of logcats we print (filtering out the useless stuff)
Attachment #677159 - Flags: review?(jmaher)
(In reply to William Lachance (:wlach) from comment #8)
> Created attachment 677159 [details] [diff] [review]
> Print logcat in talos even on error
> 
> This is a pretty seperate patch from what I was working on earlier, but it
> would be a big win for debuggability of talos tests (as gbrown pointed out,
> not printing out the logcat on failure really reduces usefulness). 
> 
> It will become even more useful if/when we expand the amount of logcats we
> print (filtering out the useless stuff)

Try run for patch here:

https://tbpl.mozilla.org/?tree=Try&rev=bfeefce801d0

(note the helpful logcats that now appear in red jobs)
Comment on attachment 677159 [details] [diff] [review]
Print logcat in talos even on error

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

ahh, good stuff.
Attachment #677159 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #10)
> Comment on attachment 677159 [details] [diff] [review]
> Print logcat in talos even on error
> 
> Review of attachment 677159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ahh, good stuff.

Pushed: http://hg.mozilla.org/build/talos/rev/a525401cc575
This is essentially the same patch I posted earlier (which got f+) with some minor errors corrected after running it through try (see above).

If this looks ok we can commit it and hopefully get some better debugging information in our tests!
Attachment #674831 - Attachment is obsolete: true
Attachment #680758 - Flags: review?(gbrown)
Comment on attachment 680758 [details] [diff] [review]
Print all the logcats (with a bit filtering)

LGTM
Attachment #680758 - Flags: review?(gbrown) → review+
Depends on: 811361
Talos part in production.

Are we all done here now? (Presume the [leave open] was just so we waited for the talos part?)
Target Milestone: --- → mozilla19
(In reply to Ed Morley [:edmorley UTC+0] from comment #16)
> Talos part in production.
> 
> Are we all done here now? (Presume the [leave open] was just so we waited
> for the talos part?)

Yes, I think we can safely resolve this.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.