1087251 - Gather version and device information for HTML log formatter

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Once we have a new version of mozlog with fixes for the HTML formatter we need to gather the version and device information within the Marionette test runner.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gather version and device information for HTML log formatter. v1.0

Comment 2

[James Graham [:jgraham]]

Comment on attachment 8509523 [details] [diff] [review]
Gather version and device information for HTML log formatter. v1.0

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

I'm going to punt this to Chris, although I think there's nothing particularly complex here he touched this code more recently than me.

My only comment is that I prefer alignment like

foo.bar(arg1="some long thing",
        arg2="something else")

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8509523 [details] [diff] [review]
Gather version and device information for HTML log formatter. v1.0

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

r+ because I think this is probably safe and I'm really glad to see mozlog being used for this. I'd love to see the nits addressed, but feel free to ignore if you think they're not pertinent.

I agree with James on argument alignment (no blank line after the opening paren).

::: testing/marionette/client/marionette/runner/base.py
@@ +22,5 @@
>  from mozhttpd import MozHttpd
>  from mozlog.structured.structuredlog import get_default_logger
>  from moztest.adapters.unit import StructuredTestRunner, StructuredTestResult
>  from moztest.results import TestResultCollection, TestResult, relevant_line
> +import mozversion

This should go above with other absolute imports in the file.

@@ +664,5 @@
>              self.add_test(test)
>  
> +        version_info = mozversion.get_version(
> +            binary=self.bin, sources=self.sources,
> +            dm_type=os.environ.get('DM_TRANS', 'adb'))

We're always running this code now where before it was only for b2g. It's probably fine, but I'd worry a little about gotchas in mozversion or confusing output for the desktop case.

@@ +668,5 @@
> +            dm_type=os.environ.get('DM_TRANS', 'adb'))
> +
> +        device_info = None
> +        if self.capabilities['device'] != 'desktop' and \
> +                self.capabilities['browserName'] == 'B2G':

I'm a big fan of using python's implicit line joining rather than '\':

if (self.capabilities['device'] != 'desktop' and
    self.capabilities['browserName'] == 'B2G'):

@@ +683,5 @@
>              self.logger.test_start(name)
>              self.logger.test_end(name,
>                                   'SKIP',
>                                   message=test['disabled'])
>              self.todo += 1

Between this loop and your addition I think we're ready to extract a "log preamble" method. It's your call, but huge methods (particularly with names like "run_tests") that accumulate disparate additions over time are a pet peeve of mine.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gather version and device information for HTML log formatter. v1.1

Carrying r+ with some items addressed. Holding off marking as checkin-needed until the dependencies are resolved.

(In reply to Chris Manchester [:chmanchester] from comment #3)
> Comment on attachment 8509523 [details] [diff] [review]
> Gather version and device information for HTML log formatter. v1.0
> 
> Review of attachment 8509523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ because I think this is probably safe and I'm really glad to see mozlog
> being used for this. I'd love to see the nits addressed, but feel free to
> ignore if you think they're not pertinent.
> 
> I agree with James on argument alignment (no blank line after the opening
> paren).

Fixed

> ::: testing/marionette/client/marionette/runner/base.py
> @@ +22,5 @@
> >  from mozhttpd import MozHttpd
> >  from mozlog.structured.structuredlog import get_default_logger
> >  from moztest.adapters.unit import StructuredTestRunner, StructuredTestResult
> >  from moztest.results import TestResultCollection, TestResult, relevant_line
> > +import mozversion
> 
> This should go above with other absolute imports in the file.

I've organised the imports according to PEP8, If I moved this to the absolute imports it would be grouped with the standard library imports rather than the third party imports. Within this group the imports are ordered alphabetically.

> @@ +664,5 @@
> >              self.add_test(test)
> >  
> > +        version_info = mozversion.get_version(
> > +            binary=self.bin, sources=self.sources,
> > +            dm_type=os.environ.get('DM_TRANS', 'adb'))
> 
> We're always running this code now where before it was only for b2g. It's
> probably fine, but I'd worry a little about gotchas in mozversion or
> confusing output for the desktop case.

This code should be harmless - in fact beneficial - to run on desktop.

> @@ +668,5 @@
> > +            dm_type=os.environ.get('DM_TRANS', 'adb'))
> > +
> > +        device_info = None
> > +        if self.capabilities['device'] != 'desktop' and \
> > +                self.capabilities['browserName'] == 'B2G':
> 
> I'm a big fan of using python's implicit line joining rather than '\':

I kept this all on one line instead. :)

> if (self.capabilities['device'] != 'desktop' and
>     self.capabilities['browserName'] == 'B2G'):
> 
> @@ +683,5 @@
> >              self.logger.test_start(name)
> >              self.logger.test_end(name,
> >                                   'SKIP',
> >                                   message=test['disabled'])
> >              self.todo += 1
> 
> Between this loop and your addition I think we're ready to extract a "log
> preamble" method. It's your call, but huge methods (particularly with names
> like "run_tests") that accumulate disparate additions over time are a pet
> peeve of mine.

I see your point, but couldn't come up with something I liked. If you have a concrete suggestion I'd be happy to implement it.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Added dependency for latest mozlog version, and triggered a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2ba82980d17b

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b212f882730

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I didn't mean to close this, it's still waiting on merge to m-c.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b212f882730