1071219 - Issues with Gaia classes

Status: RESOLVED WONTFIX

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Armen [:armenzg]]

There are various places where we repeat the same code because there are some design issues with GaiaTest and GaiaMixin.

For example:
* marionette.py instead of extending query_abs_dirs() from GaiaTest, it overwrites it and duplicates the code [1]
* the same in here but with pull() [2]

I was trying to write MuletUnitTest and I found it very hard to figure out the right way without having to do a major refactoring.

There are too many scripts that use this class and I do not want to derail re-factoring and re-testing each.


[1] http://hg.mozilla.org/build/mozharness/file/default/scripts/marionette.py#l206
http://hg.mozilla.org/build/mozharness/file/0f41dc44292e/mozharness/mozilla/testing/gaia_test.py#l164

[2]
http://hg.mozilla.org/build/mozharness/file/0f41dc44292e/mozharness/mozilla/testing/gaia_test.py#l137


armenzg-thinkpad mozharness hg:[default!] $ grep -r "GaiaTest" scripts
scripts/gaia_integration.py:from mozharness.mozilla.testing.gaia_test import GaiaTest
scripts/gaia_integration.py:class GaiaIntegrationTest(GaiaTest):
scripts/gaia_integration.py:      GaiaTest.__init__(self, require_config_file)
scripts/gaia_build_unit.py:from mozharness.mozilla.testing.gaia_test import GaiaTest
scripts/gaia_build_unit.py:class GaiaBuildUnitTest(GaiaTest):
scripts/gaia_build_unit.py:      GaiaTest.__init__(self, require_config_file)
scripts/gaia_build_integration.py:from mozharness.mozilla.testing.gaia_test import GaiaTest
scripts/gaia_build_integration.py:class GaiaBuildIntegrationTest(GaiaTest):
scripts/gaia_build_integration.py:      GaiaTest.__init__(self, require_config_file)
scripts/gaia_unit.py:from mozharness.mozilla.testing.gaia_test import GaiaTest
scripts/gaia_unit.py:class GaiaUnitTest(GaiaTest):
scripts/gaia_unit.py:        GaiaTest.__init__(self, require_config_file)
scripts/gaia_linter.py:from mozharness.mozilla.testing.gaia_test import GaiaTest
scripts/gaia_linter.py:class GaiaIntegrationTest(GaiaTest):
scripts/gaia_linter.py:      GaiaTest.__init__(self, require_config_file)
armenzg-thinkpad mozharness hg:[default!] $ grep -r "GaiaMixin" scripts
scripts/marionette.py:from mozharness.mozilla.gaia import GaiaMixin
scripts/marionette.py:                     MercurialScript, BlobUploadMixin, TransferMixin, GaiaMixin):

Comment 1

[Armen [:armenzg]]

Attachment: mozharness_refactor.diff

This does not break marionette, gaia unit and 
I'm re-testing on Ash.

NOTE: This review does not block me for Mulet work.

PS: Don't kill me and there is no rush for this review. You can even get back to me little by little.

Comment 2

[Armen [:armenzg]]

Comment on attachment 8497560 [details] [diff] [review]
mozharness_refactor.diff

I missed something for b2g desktop jobs.
I will post a new patch tomorrow or EOD.

Comment 3

[Armen [:armenzg]]

Attachment: mozharness_refactor.diff

Comment 4

[Jordan Lund (:jlund)]

(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #3)
> Created attachment 8498860 [details] [diff] [review]
> mozharness_refactor.diff

I'll be looking at this by EOD

Comment 5

[Jordan Lund (:jlund)]

Comment on attachment 8498860 [details] [diff] [review]
mozharness_refactor.diff

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

some of the end leaf scripts like mulet/marionette really show what we gain from your efforts.

It is impossible for me to dump this all in my head via a patch. For that reason, we should test this patch heavily. I think we will also have to keep a close eye on things like gaia-try branch as it uses gaiamixin/gaiatest but other classes too that are not run in cypress/ash/cedar.

It would be nice to have tests that can just tell us this is, essentially, a no-op :(

r+ if you can justify my questions/concerns. but I think we may need a small interdiff follow up patch.

::: mozharness/base/script.py
@@ +467,5 @@
>                  for line in contents.splitlines():
>                      self.info(" %s" % line)
>              return contents
>  
> +    def _query_binary_version(self, regex, cmd):

I feel like this method is much more generic than finding a binary version. maybe this should be get_regex_match_from_cmd()

or we could extend existing methods via 1) get_output_from_command and give it a regex param or 2) giving OutputParser a regex param in its init and availed when passed to run_command (right now it has error_list but we could extend its logic so it looks for a regex target and store it in parser.target_output)

::: mozharness/mozilla/gaia.py
@@ +30,5 @@
>  ]
>  
>  class GaiaMixin(object):
>  
> +    def extract_xre(self, filename, parent_dir=None):

++

@@ -300,5 @@
>          self.run_command(cmd,
>                           cwd=gaia_dir,
>                           halt_on_failure=True)
> -
> -    def make_node_modules(self):

what do we gain from moving this and node_setup() to GaiaTest?

Is it simply that they will not be used by anything outside of Gaiatest?

::: mozharness/mozilla/testing/testbase.py
@@ +309,5 @@
> +        for key in dirs.keys():
> +            if key not in abs_dirs:
> +                abs_dirs[key] = dirs[key]
> +
> +        self.abs_dirs = abs_dirs

unit.sh warns me about this. I think it's because this isn't part of self by the time this is hit. I think it's 'safe' but maybe we should put it at the top of TestingMixing with the other classattrs and default it to None

::: scripts/b2g_desktop_unittest.py
@@ +107,5 @@
>          if suite not in self.test_suites:
>              self.fatal("Don't know how to run --test-suite '%s'!" % suite)
>  
> +    def pull(self, **kwargs):
> +        GaiaMixin.pull(self, **kwargs)

why do we need to define a pull here? is it because when we call self.pull() as an action, it points somewhere else and we need to force it to use GaiaMixin? If that's the case, something bigger is wrong

@@ +123,3 @@
>          for d in self.test_suites + ('config', 'certs'):
>              dirs['abs_%s_dir' % d] = os.path.join(
> +                    abs_dirs['abs_test_install_dir'], d)

was this busted before? or was it just duplicate keys pointing to the same thing?

@@ +228,5 @@
>                                                    'reftests', 'reftest.list')
>  
> +        # In b2g_desktop jobs we can't know the self.binary_path from the beginning
> +        if not self.gaia_profile:
> +            self.gaia_profile = os.path.join(

how come this is sometimes dirs[abs_gaia_profile][1]
which doesn't seem to be given anything with self.binary_path, and times like above it does use self.binary_path?


[1] http://hg.mozilla.org/build/mozharness/file/b59a1a06dd1e/scripts/mulet_unittest.py#l42

::: scripts/marionette.py
@@ +190,1 @@
>          dirs['abs_blob_upload_dir'] = os.path.join(abs_dirs['abs_work_dir'], 'blobber_upload_dir')

abs_blob_upload_dir is in GaiaMixin._update_abs_dirs now right?

::: scripts/mulet_unittest.py
@@ +46,5 @@
>      def run_tests(self):
>          """
>          Run the unit test suite.
>          """
> +        abs_dirs = super(MuletUnittest, self).query_abs_dirs()

I think this can just be self.query_abs_dirs(). super and this should both just find the first one in MRO

Comment 6

[Armen [:armenzg]]

I will get back to this once we have mozharness try nailed down when it comes to patches.

Thanks Jordan! I will get back to you later.

Comment 7

[Armen [:armenzg]]

Not working on this.