820429 - Refactor b2g automation to avoid code duplication

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

There some duplicated code in marionette.py and b2g_emulator_unittest.py. We should pull that out into a B2GMixin to avoid code duplication.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Add remotebase.py

First stab at refactoring emulator/marionette.py code. Pretty sure this could be better, not entirely sure how ;)

I don't think remotebase.py is the best place for 'run_command_and_retry' or 'RetryParserMixin'.. but I don't know where else would be best for them to live so I put them there for now.

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 690923 [details] [diff] [review]
Patch 1.0 - Add remotebase.py

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

In general this looks fine (details of where this or that should go notwithstanding, which I'm sure aki will have an opinion on), but I don't think we should eliminate the TBPL_RETRY code entirely; we probably want to preserve that at least optionally.

Comment 3

[Aki Sasaki (not active)]

Comment on attachment 690923 [details] [diff] [review]
Patch 1.0 - Add remotebase.py

Hm, do we really need so many additional mixins?
mozharness.mozilla.testing.unittest.EmulatorMixin could potentially be rolled into one of these, though I'm not sure which one would be preferable.

>+    def run_command_and_retry(self, command, retry_condition, retries=3, cwd=None,

I think run_command_and_retry() could go next to (or inside of?) run_command(), though I think people in releng were asking for something similar to retry() in tools:
http://hg.mozilla.org/build/tools/file/0fc4f78ca089/lib/python/util/retry.py#l8

I think either approach would require some thinking about what happens if we hit halt_on_failure or fatal().

I'm ok with this landing without solving everyone's use case, but it'll probably get rewritten in the future.

>+            if not retry_condition() or retry == retries:
>+                break
>+            self.info("Retrying command (attempt %s of %s)" % (retry, retries))+        return return_code

I think there should be some message here that we've run out of retries, and it's still not working.

>+class RetryParserMixin():

I'd be okay with this change landing in OutputParser proper.

We could add another key to the error_list dicts, 'retry'.
This could be a bool or an enum (None / 'command' / 'job') where 'command' signals the specific command should be retried, and 'job' signals a need to restart the entire script.

The above approach would obsolete the need for MarionetteUnittestOutputParser.

I've been thinking about built-in retries for a bit so we can discuss this.

>-from mozharness.base.errors import BaseErrorList
>+from mozharness.base.errors import PythonErrorList

I know Jordan hit a lot of false positives when he used PythonErrorList for unittests.  I'm going to guess that marionette is a lot cleaner with its output than the other unittest harnesses; just highlighting this as something that might do the same.

If it doesn't cause false positives, this is a good thing to do :)


Seems like we should discuss retries at the very least.  Clearing the r? flag til then.

Comment 4

[Andrew Halberstadt [:ahal]]

(In reply to Aki Sasaki [:aki] from comment #3)
> I'm ok with this landing without solving everyone's use case, but it'll
> probably get rewritten in the future.

I'm not in any rush to land this, just wanted to get the ball rolling. I'll get another patch up when I have some free cycles. Let's get this done right the first time :)

Comment 5

[Andrew Halberstadt [:ahal]]

I'm not actively working on this anymore.

Comment 6

[Rail Aliiev [:rail]]

Andrew, is there anything that we still want address in the bug?

Comment 7

[Andrew Halberstadt [:ahal]]

Nope, I'm no longer convinced doing this is even a good idea.. let's WONTFIX.