1200368 - Clean-up scripts for firefox-ui-tests

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The current code needs some clean-up to follow the general style and behavior, and especially to allow us to create other scripts for other test types we have beside the update tests.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Clean-up patch v1

So this WIP changes a lot! It refactors the code of the scripts so it can easily be re-used for our functional tests. Therefore I had to move/rewrite a fair amount of the code. Extensive testing has shown that all seems to work pretty fine so far. Armen, if possible I would like that you also test with a non developer config. 

Here the changes:
* Refactored the code of FirefoxUIUpdates and FirefoxUITests classes to make it easier to add new test types (scripts)
* Tried to use more generally available methods and data instead of locally hard-coded ones.
* Set new default actions which exclude the release update config action and which has to be explicitly added via the command line.
* Fixed bug in limit-locales introduced by my patch on bug 1199202
* Changed handling of env variables - we no longer ignore them all from the host (necessary for e.g. proxy settings of our CI machines)
* Fixed double usage of --update-channel in case of a release update config and cli option


The crash symbols logic still needs to be improved. But that I want to do on another bug because it might need changes to some base mozharness files and I don't want to bring in more changes to this patch.

Comment 2

[Armen [:armenzg]]

Hi whimboo,
With regards to non developer config, you can follow these steps to make it run on the releng infra without it:
https://wiki.mozilla.org/Auto-tools/Projects/Marionette_update_tests#Test_your_changes_on_the_Try_server

You would need two pushes, one for 32-bit builds and another for 64-bit builds.
Let me know if you have any questions.

I am hoping to have the review in the next 2 hours.

Comment 3

[Armen [:armenzg]]

Comment on attachment 8655597 [details] [diff] [review]
Clean-up patch v1

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

Hi Henrik,
I'm sorry if I lead you to believe that this is the type of changes I wanted to see (I don't recall if this is what we spoke about and might have misunderstood).

Unfortunately, I would like to keep every thing that has to do with "update tests" (e.g. tools repo) in firefox_ui_updates.py
Anything that is common to all Firefox UI tests (updates, functionals and the other one) can stay in the base class.

Please let me know if there are reasons that I'm missing to move so much of the code up to the base class.
I see some code it makes sense to move to the base class, however, I don't see the reason for most.
Perhaps it would be easier to understand once I see the other subclassing scripts.

The architectural design of having the base class was to allow creating various scripts that would subclass the base class.

::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ +22,5 @@
>      VirtualenvMixin,
>      virtualenv_config_options,
>  )
> +from mozharness.mozilla.buildbot import TBPL_SUCCESS, TBPL_WARNING, EXIT_STATUS_DICT
> +from mozharness.mozilla.testing.testbase import (INSTALLER_SUFFIXES)

Why between brackets? I'm curious.

@@ +27,4 @@
>  from mozharness.mozilla.vcstools import VCSToolsScript
>  
>  
> +firefoxuitests_config_options = [

I would do "firefox_ui_tests_config_options"
It doesn't matter enough.

@@ +58,5 @@
> +    }],
> +    [['--tools-tag'], {
> +        'dest': 'tools_tag',
> +        'help': 'Which revision/tag to use for the tools repository.',
> +    }],

Do we want to move the tools flag this high up in the hierarchy? (Since it is only used for release update tests).

@@ +107,5 @@
> +                                config_options=config_options,
> +                                all_actions=all_actions or actions,
> +                                default_actions=default_actions or actions,
> +                                **kwargs)
> +        VirtualenvMixin.__init__(self)

Interesting way of initializing... no complains just different than what I've seen.

@@ +112,5 @@
>  
> +        self.limit_locales = int(self.config.get('limit_locales'))
> +
> +        self.tools_repo = self.config.get('tools_repo')
> +        self.tools_tag = self.config.get('tools_tag')

Only applicable if running release update tests.

@@ +135,4 @@
>  
> +        # This will be a list containing one item per release based on configs
> +        # from tools/release/updates/*cfg
> +        self.releases = None

Only applicable if running release update tests.

@@ +145,5 @@
>              'firefox-ui-tests',
>              url=dirs['fx_ui_dir'],
>          )
>  
> +    def _modify_url(self, rel_info):

This is very specific to releases and update tests.

@@ +188,5 @@
> +        else:
> +            self.fatal('Can\'t find symbols_url from installer_url: {}!'.format(installer_url))
> +
> +    def _run_test(self, installer_path, script_name, symbols_url=None,
> +                  cleanup=True, marionette_port=2828):

This is specific to update tests (not necessarily release update tests).

@@ +257,5 @@
> +                repo=self.tools_repo,
> +                dest=dirs['abs_tools_dir'],
> +                revision=self.tools_tag,
> +                vcs='hgtool'
> +            )

It only makes sense for release update tests.

@@ +265,5 @@
> +            return self.abs_dirs
> +
> +        abs_dirs = super(FirefoxUITests, self).query_abs_dirs()
> +        abs_dirs.update({
> +            'abs_tools_dir': os.path.join(abs_dirs['abs_work_dir'], 'tools'),

abs_tools_dir only makes sense for release update tests.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I chatted with Armen earlier today and we agreed on that most of the code related to release configs will be moved back. We might not have any configs for release builds to test via e.g. functional tests. But if we do they might look different and we would have to separate the code anyway. Also it might be worth to have two different update scripts - one for nightly builds and one for releases. It would clean-up the code even more.

For now I will also move the symbol handling code back to the release update class, and have a follow-up to get it properly fixed.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Clean-up patch v2

So I moved back all release related code to the now separated script. All basic behavior which corresponds to all available test types remains in the firefox-ui-tests.py file.

I haven't tested this on try yet, but will do when I'm back later today. Locally all works fine for me.

Comment 6

[Armen [:armenzg]]

Comment on attachment 8656524 [details] [diff] [review]
Clean-up patch v2

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

This looks very good.

I have a bunch of questions and comments for you to go through.
This would pretty much be an r+ by fixing the nits, however, I would like to see one last time the interdiff since this is a pretty large change.

If you want to land this either Friday or Monday when I'm away feel free to land it as an r+, however, I would like to see the interdiff later on.

::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ +28,5 @@
> +firefox_ui_tests_config_options = [
> +    [['--build-number'], {
> +        'dest': 'build_number',
> +        'help': 'Build number of release, eg: 2',
> +    }],

Is --build-number used also for non-update tests?
If it makes sense, I would put this in firefox_ui_update_harness_config_options.
If it's shared, nvm!

@@ +103,2 @@
>  
> +    cli_script = 'firefox-ui-tests'

With my patch in bug 1198448, this might be update_script.py or tests_script.py (pending review for naming).

I would nevertheless start with None value and let the derived classes set it.

FTR, this is not a request but what I think I would do (aka suggestion).

For reference:
https://github.com/armenzg/firefox-ui-tests/commit/f8bd716ba830fc921117db3db2f5d6d993337b1b

@@ +147,5 @@
>              'firefox-ui-tests',
>              url=dirs['fx_ui_dir'],
>          )
>  
> +    def _query_symbols_url(self, installer_url):

Could you please file a bug about what you mentioned about symbols url? (since it is in your head space and can probably word it better) Thanks!

@@ +169,5 @@
> +                return None
> +        else:
> +            self.fatal('Can\'t find symbols_url from installer_url: {}!'.format(installer_url))
> +
> +    def _run_test(self, installer_path, script_name, symbols_url=None,

As we work more on the derived classes we might need to re-adjust this base function on how to run a test.

Perhaps we should not start the name of the function with "_" to indicate that it is not a private function anymore and it is OK to derive.

@@ +192,5 @@
> +
> +        if symbols_url:
> +            cmd += ['--symbols-path', symbols_url]
> +
> +        cmd.extend(self.query_extra_cmd_args())

Could you please add a comment as to why we call this method?

@@ +195,5 @@
> +
> +        cmd.extend(self.query_extra_cmd_args())
> +
> +        return_code = self.run_command(cmd, cwd=dirs['abs_work_dir'],
> +                                       output_timeout=300)

I need use env in order to make use of the MINIDUMP_STACKWALK (e.g. https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/releng_infra_configs/linux64.py)

In fact, we need to enforce the usage of MINIDUMP_STACKWALK if symbols_url is not None.

@@ +253,5 @@
> +        dirs = self.query_abs_dirs()
> +
> +        if not self.installer_path and not self.installer_url:
> +            self.critical('Please specify an installer via --installer-path or --installer-url.')
> +            sys.exit(1)

We can problably put this block in a _pre_run_tests() action plus @PreScriptAction('run-tests')

@@ +265,5 @@
> +        symbols_url = self._query_symbols_url(installer_url=self.installer_path)
> +
> +        # We don't want multiple outputs of the same environment information. To prevent
> +        # that, we can't make it an argument of run_command and have to print it on our own.
> +        self.info('Using env: {}'.format(pprint.pformat(self.query_env())))

When we call _run_test() of the base class we don't pass "env" to the run command.

Do we need to output the env in here?

In any case, this line probably makes more sense to live within the _run_test method().

@@ +285,5 @@
> +
> +        FirefoxUITests.__init__(self, config_options=config_options,
> +                                *args, **kwargs)
> +
> +    def query_extra_cmd_args(self):

Could you please have a brief description of what this method does?

::: testing/mozharness/scripts/firefox_ui_update.py
@@ +2,1 @@
>  # ***** BEGIN LICENSE BLOCK *****

Are you thinking of making this script the one used for nightly jobs?
Or the simplest script to test a single update test? (and have a different nightly script)?

::: testing/mozharness/scripts/firefox_ui_update_release.py
@@ +1,1 @@
> +#!/usr/bin/env python

Can you please put the firefox ui scripts under scripts/firefox_ui_tests/ ?
Now that we have more than one script we should keep the scripts root dir clean (I perhaps should have started in the same way).

@@ +60,5 @@
> +    }],
> +] + copy.deepcopy(firefox_ui_update_config_options)
> +
> +
> +class ReleaseFirefoxUIUpdateTests(FirefoxUIUpdateTests):

This split up is looking nice!

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Armen Zambrano Gasparnian [:armenzg] from comment #6)
> ::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
> @@ +28,5 @@
> > +firefox_ui_tests_config_options = [
> > +    [['--build-number'], {
> > +        'dest': 'build_number',
> > +        'help': 'Build number of release, eg: 2',
> > +    }],
> 
> Is --build-number used also for non-update tests?
> If it makes sense, I would put this in
> firefox_ui_update_harness_config_options.
> If it's shared, nvm!

Yes, we do when we have to run our functional tests for beta/release candidate builds. But lets look at this later. To ease the clean-up I will keep it for release updates for now. Also it would go away anyway because for release update tests we will use symbols of the source build, which does not need a build number.

> > +    cli_script = 'firefox-ui-tests'
> 
> With my patch in bug 1198448, this might be update_script.py or
> tests_script.py (pending review for naming).
> 
> I would nevertheless start with None value and let the derived classes set
> it.

Derived classes like the update tests are setting it, please see the lines of code in the other file. If a derived class does NOT set it, the script from the parent class is taken. I don't see a reason to have it not as a class variable. It's not bound to a class instance.

> For reference:
> https://github.com/armenzg/firefox-ui-tests/commit/
> f8bd716ba830fc921117db3db2f5d6d993337b1b

I might have to also look at this and figure out a better way. I'm not that comfortable in having those files in our test repository.

> @@ +147,5 @@
> >              'firefox-ui-tests',
> >              url=dirs['fx_ui_dir'],
> >          )
> >  
> > +    def _query_symbols_url(self, installer_url):
> 
> Could you please file a bug about what you mentioned about symbols url?
> (since it is in your head space and can probably word it better) Thanks!

Filed bug 1201588.

> @@ +169,5 @@
> > +                return None
> > +        else:
> > +            self.fatal('Can\'t find symbols_url from installer_url: {}!'.format(installer_url))
> > +
> > +    def _run_test(self, installer_path, script_name, symbols_url=None,
> 
> As we work more on the derived classes we might need to re-adjust this base
> function on how to run a test.
> 
> Perhaps we should not start the name of the function with "_" to indicate
> that it is not a private function anymore and it is OK to derive.

Right now I don't see a need that derived classes will have to override it but sure, lets make it a public method. I feel that the query_extra_cmd_args() method could serve this instead.

> @@ +192,5 @@
> > +
> > +        if symbols_url:
> > +            cmd += ['--symbols-path', symbols_url]
> > +
> > +        cmd.extend(self.query_extra_cmd_args())
> 
> Could you please add a comment as to why we call this method?

Done.

> @@ +195,5 @@
> > +
> > +        cmd.extend(self.query_extra_cmd_args())
> > +
> > +        return_code = self.run_command(cmd, cwd=dirs['abs_work_dir'],
> > +                                       output_timeout=300)
> 
> I need use env in order to make use of the MINIDUMP_STACKWALK (e.g.
> https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/
> releng_infra_configs/linux64.py)
> 
> In fact, we need to enforce the usage of MINIDUMP_STACKWALK if symbols_url
> is not None.

Hm, given that this variable gets set inside the script and it not globally, we have to pass-in the environment to the run_command() method. This also means that we will get a full print out of the environment for each build. Is that correct?

> @@ +253,5 @@
> > +        dirs = self.query_abs_dirs()
> > +
> > +        if not self.installer_path and not self.installer_url:
> > +            self.critical('Please specify an installer via --installer-path or --installer-url.')
> > +            sys.exit(1)
> 
> We can problably put this block in a _pre_run_tests() action plus
> @PreScriptAction('run-tests')

Sure, good call. Totally missed that.

> @@ +265,5 @@
> > +        symbols_url = self._query_symbols_url(installer_url=self.installer_path)
> > +
> > +        # We don't want multiple outputs of the same environment information. To prevent
> > +        # that, we can't make it an argument of run_command and have to print it on our own.
> > +        self.info('Using env: {}'.format(pprint.pformat(self.query_env())))
> 
> When we call _run_test() of the base class we don't pass "env" to the run
> command.
> 
> Do we need to output the env in here?
> 
> In any case, this line probably makes more sense to live within the
> _run_test method().

That's what we talked about yesterday in our Vidyo chat. If we pass in the env to run_command the full environment will be printed out for each build. That's something I tried to solve here. But given your above comment we will have internally set env variables, so this needs to be reverted.

> @@ +285,5 @@
> > +
> > +        FirefoxUITests.__init__(self, config_options=config_options,
> > +                                *args, **kwargs)
> > +
> > +    def query_extra_cmd_args(self):
> 
> Could you please have a brief description of what this method does?

Done.

> ::: testing/mozharness/scripts/firefox_ui_update.py
> @@ +2,1 @@
> >  # ***** BEGIN LICENSE BLOCK *****
> 
> Are you thinking of making this script the one used for nightly jobs?
> Or the simplest script to test a single update test? (and have a different
> nightly script)?

This will be the script for nightly update tests, right. For functional tests we will have firefox_ui_functional.py soon.

> ::: testing/mozharness/scripts/firefox_ui_update_release.py
> @@ +1,1 @@
> > +#!/usr/bin/env python
> 
> Can you please put the firefox ui scripts under scripts/firefox_ui_tests/ ?
> Now that we have more than one script we should keep the scripts root dir
> clean (I perhaps should have started in the same way).

Makes sense. I may do this in the over-next version of the patch so you get a better interdiff of the changes.

New patch is coming in a bit.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Clean-up patch v3

Intermediate patch without moving the scripts for a better interdiff. The next patch will be the final version.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Clean-up patch v3.1

Taking over r+ but requesting another feedback just to make sure you are happy now, Armen.

Comment 10

[Armen [:armenzg]]

Another comment will follow this one.

(In reply to Henrik Skupin (:whimboo) from comment #7)
> (In reply to Armen Zambrano Gasparnian [:armenzg] from comment #6)
> > ::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
> > @@ +195,5 @@
> > > +
> > > +        cmd.extend(self.query_extra_cmd_args())
> > > +
> > > +        return_code = self.run_command(cmd, cwd=dirs['abs_work_dir'],
> > > +                                       output_timeout=300)
> > 
> > I need use env in order to make use of the MINIDUMP_STACKWALK (e.g.
> > https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/
> > releng_infra_configs/linux64.py)
> > 
> > In fact, we need to enforce the usage of MINIDUMP_STACKWALK if symbols_url
> > is not None.
> 
> Hm, given that this variable gets set inside the script and it not globally,
> we have to pass-in the environment to the run_command() method. This also
> means that we will get a full print out of the environment for each build.
> Is that correct?
> 
When you say each build, are you meaning each locale we process? if that is the case, yes.

This is why I used avoid_host_env.

Comment 11

[Armen [:armenzg]]

Comment on attachment 8656745 [details] [diff] [review]
Clean-up patch v3.1

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

Thank you for the interdiff!

For reference: https://bugzilla.mozilla.org/attachment.cgi?oldid=8656524&action=interdiff&newid=8656738&headers=1

Feel free to land without a try run. DONTBUILD or NPOTB to be used.

::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ +143,5 @@
>              url=dirs['fx_ui_dir'],
>          )
>  
> +    @PreScriptAction('run-tests')
> +    def _pre_run_tests(self, action):

Do you prefer this method far from the actual method which is associated to?
Are you using alphabetical sorting of methods?

::: testing/mozharness/scripts/firefox_ui_tests/update_release.py
@@ +16,5 @@
> +import sys
> +import urllib
> +
> +# load modules from parent dir
> +sys.path.insert(1, os.path.dirname(os.path.dirname(sys.path[0])))

Good!

@@ +272,5 @@
> +
> +                retcode = self.run_test(
> +                    installer_path=installer_path,
> +                    script_name=self.cli_script,
> +                    env=self.query_env(avoid_host_env=True),

Woohoo!

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8656745 [details] [diff] [review]
Clean-up patch v3.1

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

::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ +143,5 @@
>              url=dirs['fx_ui_dir'],
>          )
>  
> +    @PreScriptAction('run-tests')
> +    def _pre_run_tests(self, action):

Good point. I'm using alphabetical sorting but in this case it should be better to place the pre action methods right next to the real action. I will fix that.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Clean-up patch v4

Final patch with fixed recent review comments. Taking over r+ and ready to get landed.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be8c33fb9849

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/be8c33fb9849