Closed Bug 1445944 Opened 6 years ago Closed 6 years ago

support running tests against google chrome in our CI

Categories

(Testing :: Mozbase, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: ahal)

References

Details

(Whiteboard: [PI:April])

Attachments

(11 files, 9 obsolete files)

43 bytes, text/x-github-pull-request
rwood
: review+
Details | Review
43 bytes, text/x-github-pull-request
rwood
: review+
Details | Review
59 bytes, text/x-review-board-request
rwood
: review+
Details
59 bytes, text/x-review-board-request
rwood
: review+
Details
59 bytes, text/x-review-board-request
davehunt
: review+
Details
59 bytes, text/x-review-board-request
rwood
: review+
Details
59 bytes, text/x-review-board-request
davehunt
: review+
Details
59 bytes, text/x-review-board-request
rwood
: review+
Details
59 bytes, text/x-review-board-request
rwood
: review+
Details
59 bytes, text/x-review-board-request
rwood
: review+
Details
59 bytes, text/x-review-board-request
rwood
: review+
Details
we want to run talos tests against google chrome so we have some form of competitive benchmarking.  This means we need to:
1) obtain a nightly version of google chrome (some prior art in AWFY: https://github.com/mozilla/arewefastyet/blob/master/slave/download.py#L272)
2) setup profile and launch process (prior art for process: https://github.com/mozilla/arewefastyet/blob/master/slave/executors.py#L154, also web-platform-tests has prior art)
3) integration this into talos so we can run with a path to the binary and --google-chrome mode for managing preferences/env_vars/process/web_extension/reporting
4) add support in taskcluster to run tests (tp6, speedometer and motionmark jobs) with google chrome.  I am not sure if we need a download job to obtain and cache the google-chrome nightly build.
5) schedule this to run on nightly builds on mozilla-central.
6) figure out how to post results to perfherder- since we don't have support for other products, I would recommend adding an option to existing tests as 'chrome' or  'google' so we would upload results for "tp6 stylo e10s google-chrome opt" and that should work with our existing toolchains.

I am sure there are details I have overlooked, likewise some details I have added which are incorrect or not needed.
No longer blocks: 1442289
Blocks: 1445952
Assignee: nobody → ahalberstadt
Blocks: 1442289
Whiteboard: [PI:March]
I have a couple of questions:

1) Should this be integrated with Raptor instead of Talos? If we plan to slowly start deprecating Talos and write all new tests in Raptor, then I think we'd get more value focusing there.

2) Should we build this with the wpt infrastructure or the AWFY infrastructure? The wpt implementation looks more robust, but depends heavily on webdriver. Is using webdriver an explicit non-goal? Would we ever conceivably want to share these tests with other vendors?

I also have some observations:

* This should be a standalone module, similar to mozrunner (or perhaps integrated into mozrunner)
* We might be able to find a module for starting/stopping chrome in the chromium source code
yes for raptor, if it isn't ready have a proof of concept for talos.  Talos will probably not go away, but will be simplified a lot.

I don't mind which wpt or awfy- whichever is better, I suspect wpt is more robust and maintained.  I have found that marionette doesn't work with talos historically, so not requiring webdriver, geckodriver, marionette would be good.

I like the idea of a standalone module or some easy addition to mozrunner.
Ok, if we don't want to require webdriver it's probably best to start off with AWFY's implementation. That will also be the simpler solution, so easier to get a prototype working.
Depends on: 1447401
I had another clarification. The bullet list from comment 0 doesn't mention the tests themselves supporting Chrome. Can I assume that either the tests + pageloader (or however measurements are taken) can already run with chrome? Or at least that this is being worked on in a separate bug?

As a follow-up, is there a way to run a talos test manually just so I can find one that works as something to work towards? Is there a test you'd like to see prioritized?
this will be used in the new raptor framework that :rwood is working on.  If you want to integrate into there to start that would be fine.  That framework will run the benchmarks (speedometer, jetstream,ares-6, etc.) and pageload tests (tp6) instead of talos.
Thanks for the clarification. Comment 2 made it sound like we wanted to support this in Talos as well as raptor, but I guess pageloader probably doesn't work in Chrome.

I'll sync up with Rob to figure out the state of Chrome support.
More details in the pull request, but basically this just gets things using mozrunner.

There is still no support for Chrome profiles, I'll file a follow-up to tackle that. This deletes a whole bunch of Talos code that wasn't really being used. We may need to re-implement some of it (for example running in 'debug' mode), but I think it will be easier to start from scratch than to try to shoe-horn the talos_process.py stuff into the mozrunner model.

This is a pretty large change, so please ask if you have any questions.
Attachment #8963373 - Flags: review?(rwood)
Comment on attachment 8963373 [details] [review]
Use mozrunner to manager Firefox/Chrome processes

Thanks Andrew. See comments in the PR.

I say we go ahead and merge this, and figure out the issue of killing the browser (more specifically on google Chrome, since chrome web extensions are unable to dump output to stdout to signal the output handler to kill the browser) in a future PR.
Attachment #8963373 - Flags: review?(rwood) → review+
I believe we are at the 50%+ mark on completing this- moving to April as we won't get it done in the next day or so.
Whiteboard: [PI:March] → [PI:April]
Yes, we have chrome running via mozrunner, but no support for profiles or setting prefs yet. Haven't had a chance to start looking into this, but should in the next day or two.
Status: NEW → ASSIGNED
Depends on: 1451733
Moving this to mozbase. I'll use this bug to get the mozbase specific parts landed in m-c first. The raptor parts will already be landed in github, and these mozbase pieces will start being used once that gets moved into m-c.
Component: Talos → Mozbase
Comment on attachment 8967064 [details] [review]
Use mozprofile for managing chrome profiles

LGTM and works great. Couple notes in the PR, I believe they are issues that maybe can be solved with google chrome prefs.
Attachment #8967064 - Flags: review?(rwood) → review+
Thanks for the review!

Once this is merged I'll start work on porting these changes to mozilla-central (in advance of the raptor merge). I'll call this bug resolved once the mozprofile/mozrunner changes hit m-c.
Attachment #8967887 - Attachment is obsolete: true
Attachment #8967888 - Attachment is obsolete: true
Attachment #8967889 - Attachment is obsolete: true
Attachment #8967889 - Attachment is obsolete: false
Attachment #8967888 - Attachment is obsolete: false
Attachment #8967887 - Attachment is obsolete: false
Something weird happened with that review request :/. The commit messages aren't showing up for some of the commits. I'll have to look into it next week.

Also I'll be picking some "lucky" reviewers :)
Attachment #8967890 - Attachment is obsolete: true
Attachment #8968196 - Flags: review?(rwood)
Attachment #8968197 - Flags: review?(rwood)
Attachment #8968198 - Flags: review?(dave.hunt)
Attachment #8968199 - Flags: review?(rwood)
Attachment #8968200 - Flags: review?(dave.hunt)
Attachment #8968201 - Flags: review?(rwood)
Attachment #8968202 - Flags: review?(rwood)
Attachment #8968203 - Flags: review?(rwood)
Attachment #8968204 - Flags: review?(rwood)
Attachment #8967895 - Attachment is obsolete: true
Attachment #8967887 - Attachment is obsolete: true
Attachment #8967888 - Attachment is obsolete: true
Attachment #8967889 - Attachment is obsolete: true
Attachment #8967891 - Attachment is obsolete: true
Attachment #8967892 - Attachment is obsolete: true
Attachment #8967893 - Attachment is obsolete: true
Attachment #8967894 - Attachment is obsolete: true
Sorry for all the bugspam, something went wrong with the initial push. I discarded it and pushed again and it seems to be ok now.
Comment on attachment 8968198 [details]
Bug 1445944 - [mozrunner] Convert mozrunner unittests to the pytest format

https://reviewboard.mozilla.org/r/236888/#review242678
Attachment #8968198 - Flags: review?(dave.hunt) → review+
Comment on attachment 8968196 [details]
Bug 1445944 - [moztest] Update shared test fixtures so they can work outside of mozilla-central

https://reviewboard.mozilla.org/r/236884/#review242692
Attachment #8968196 - Flags: review?(rwood) → review+
Comment on attachment 8968200 [details]
Bug 1445944 - [mozprofile] Convert mozprofile unittests to the pytest format

https://reviewboard.mozilla.org/r/236892/#review242680

::: testing/mozbase/mozprofile/tests/test_addonid.py:150
(Diff revision 1)
> +    f.write(filecontents)
> +    f.close()
> +    return path
> +
> +
> +def test_addonID(testlist):

You could use a fixture to create the install.rdf, and yield the path. This fixture could use pytest's tmpdir to manage the temporary directory. It could then be parameterised based on testlist, so the test is invoked for each rdf file instead of looping over them.

::: testing/mozbase/mozprofile/tests/manifest.ini
(Diff revision 1)
> -[server_locations.py]
> +[test_server_locations.py]
>  [test_preferences.py]
> -[permissions.py]
> -[bug758250.py]
> +[test_permissions.py]
> +[test_bug758250.py]
>  [test_nonce.py]
> -[bug785146.py]

Was this file intentionally removed?

::: testing/mozbase/mozprofile/tests/test_permissions.py:25
(Diff revision 1)
>  
> -    profile_dir = None
> -    locations_file = None
>  
> -    def setUp(self):
> -        self.profile_dir = tempfile.mkdtemp()
> +@pytest.fixture
> +def locations_file(tmpdir):

It looks like tmpdir isn't used here.

::: testing/mozbase/mozprofile/tests/test_permissions.py:40
(Diff revision 1)
> +    return Permissions(tmpdir.mkdir('profile').strpath, locations_file.name)
> +
> +
> +def test_create_permissions_db(perms):
> +    profile_dir = perms._profileDir
> +    perms_db_filename = os.path.join(profile_dir, 'permissions.sqlite')

perms_db_filename could be a fixture as it's used in a couple of places. Alternatively (or additionally) the db connection could be a fixture.

::: testing/mozbase/mozprofile/tests/test_server_locations.py:51
(Diff revision 1)
> +    assert location.host == host
> +    assert location.port == port
> +    assert location.options == options
> +
> +
> +def create_temp_file(contents):

Could we use pytest's tmpdir fixture instead?

::: testing/mozbase/mozprofile/tests/test_server_locations.py:90
(Diff revision 1)
>  
> -        self.assertRaises(BadPortLocationError, locations.add_host, '127.0.0.1',
> -                          port='abc')
> +    with pytest.raises(BadPortLocationError):
> +        locations.add_host('127.0.0.1', port='abc')
>  
> -        # test some errors in locations file
> +    # test some errors in locations file
> -        f = self.create_temp_file(self.locations_no_primary)
> +    f = create_temp_file(LOCATIONS_NO_PRIMARY)

It feels like this should be a separate test, though I appreciate that may be out of scope for this bug.

::: testing/mozbase/mozprofile/tests/test_server_locations.py:103
(Diff revision 1)
> -        self.assertEqual(exc.err.__class__, MissingPrimaryLocationError)
> -        self.assertEqual(exc.lineno, 3)
> +    assert exc.err.__class__ == MissingPrimaryLocationError
> +    assert exc.lineno == 3
>  
> -        # test bad port in a locations file to ensure lineno calculated
> +    # test bad port in a locations file to ensure lineno calculated
> -        # properly.
> +    # properly.
> -        f = self.create_temp_file(self.locations_bad_port)
> +    f = create_temp_file(LOCATIONS_BAD_PORT)

As above, I feel like this should be a separate test.

::: testing/mozbase/mozprofile/tests/test_addons.py:188
(Diff revision 1)
>  
> -        self.profile.reset()
>  
> -        self.profile.addons.install(addon)
> -        self.assertEqual(self.profile.addons.installed_addons, [addon])
> +def test_install_backup(tmpdir, profile):
> +    tmpdir = tmpdir.strpath
> +    am = profile.addons

We have this in most (if not all) of these tests. It's minor, but we could have am `am` fixture to save a few lines of duplication.

::: testing/mozbase/mozprofile/tests/test_addons.py:297
(Diff revision 1)
> -        addon_path = os.path.join(os.path.join(here, 'files'), 'not_an_addon.txt')
> +    addon_path = os.path.join(os.path.join(here, 'files'), 'not_an_addon.txt')
> -        self.assertRaises(mozprofile.addons.AddonFormatError,
> -                          self.am.addon_details, addon_path)
> +    with pytest.raises(mozprofile.addons.AddonFormatError):
> +        am.addon_details(addon_path)
> +
> +
> +@pytest.mark.skip("bug 900154")

The associated bug has since been closed.

::: testing/mozbase/mozprofile/tests/test_clone_cleanup.py:56
(Diff revision 1)
> -        clone.cleanup()
> +    clone.cleanup()
>  
> -        # clone should be deleted
> +    # clone should be deleted
> -        self.assertFalse(os.path.exists(clone.profile))
> -        self.assertTrue(counter[0] > 0)
> +    assert not os.path.exists(clone.profile)
> +    assert counter[0] > 0
> +    mozfile.remove(clone.profile)

We try to remove the directory after we've asserted that it doesn't exist. Also, if the clone is in `tmpdir` then pytest will remove it for us.

::: testing/mozbase/mozprofile/tests/test_clone_cleanup.py:66
(Diff revision 1)
> +    clone = Profile.clone(profile.profile, restore=False)
> -        clone.cleanup()
> +    clone.cleanup()
>  
> -        # clone should still be around on the filesystem
> +    # clone should still be around on the filesystem
> -        self.assertTrue(os.path.exists(clone.profile))
> +    assert os.path.exists(clone.profile)
> +    mozfile.remove(clone.profile)

If the clone is within `tmpdir` then we should be able to let pytest clean it up.

::: testing/mozbase/mozprofile/tests/test_profile_view.py:29
(Diff revision 1)
> -        keys = set(['Files', 'Path', 'user.js'])
> +    keys = set(['Files', 'Path', 'user.js'])
> -        ff_prefs = mozprofile.FirefoxProfile.preferences  # shorthand
> +    ff_prefs = mozprofile.FirefoxProfile.preferences  # shorthand
> -        pref_string = '\n'.join(['%s: %s' % (key, ff_prefs[key])
> +    pref_string = '\n'.join(['%s: %s' % (key, ff_prefs[key])
> -                                 for key in sorted(ff_prefs.keys())])
> +                             for key in sorted(ff_prefs.keys())])
>  
> -        tempdir = tempfile.mkdtemp()
> +    tempdir = tempfile.mkdtemp()

Could we use pytest's `tmpdir` fixture here? If we can, then we should also be able to remove the try/catch as pytest will handle cleaning up the temporary directory.
Attachment #8968200 - Flags: review?(dave.hunt) → review+
Blocks: 1397417
Comment on attachment 8968200 [details]
Bug 1445944 - [mozprofile] Convert mozprofile unittests to the pytest format

https://reviewboard.mozilla.org/r/236892/#review242680

> You could use a fixture to create the install.rdf, and yield the path. This fixture could use pytest's tmpdir to manage the temporary directory. It could then be parameterised based on testlist, so the test is invoked for each rdf file instead of looping over them.

Yeah, I was being a bit lazy for this (and most of the other places you recommend using fixtures below) and keeping things as is. But you're right, greater use of fixtures will make all this more readable so I'll fix it up some more.

> Was this file intentionally removed?

Yes, I probably should have done this in a separate commit to make it more obvious. But this test was moved into test_permissions.py (as it is also testing the permissions module and uses many of the same fixtures as those tests).

> The associated bug has since been closed.

I'll either enable it or xfail it.

p.s we should set the xfail_strict option globally for our tests, I'll file another bug to do that.

> We try to remove the directory after we've asserted that it doesn't exist. Also, if the clone is in `tmpdir` then pytest will remove it for us.

Good call, this should be in a finally. The clone uses `tempfile` but I guess we can set the root temp directory first. We'd still have to do cleanup to revert the root though, so probably not worth it.
Attachment #8968197 - Flags: review?(rwood) → review+
Comment on attachment 8968199 [details]
Bug 1445944 - [mozrunner] Create a base BlinkRuntimeRunner and add a ChromeRunner to the runners list

https://reviewboard.mozilla.org/r/236890/#review242752

LGTM

::: testing/mozbase/mozrunner/mozrunner/runners.py:79
(Diff revision 1)
>      return GeckoRuntimeRunner(*args, **kwargs)
>  
>  
> +def ChromeRunner(*args, **kwargs):
> +    """
> +    Create a destkop Google Chrome runner.

nit: typo
Attachment #8968199 - Flags: review?(rwood) → review+
Comment on attachment 8968200 [details]
Bug 1445944 - [mozprofile] Convert mozprofile unittests to the pytest format

https://reviewboard.mozilla.org/r/236892/#review242680

> It feels like this should be a separate test, though I appreciate that may be out of scope for this bug.

I agree with you, but I think I'd rather not make this patch more complex than it already is. I think in general our mozbase tests could use a lot of improvement.
Comment on attachment 8968201 [details]
Bug 1445944 - [mozprofile] Add a 'create_profile' helper method for instanting an instance from an app

https://reviewboard.mozilla.org/r/236894/#review242764

Cool!
Attachment #8968201 - Flags: review?(rwood) → review+
Comment on attachment 8968202 [details]
Bug 1445944 - [mozprofile] Pull functionality out of Profile and into an abstract 'BaseProfile' class

https://reviewboard.mozilla.org/r/236896/#review242766
Attachment #8968202 - Flags: review?(rwood) → review+
Comment on attachment 8968203 [details]
Bug 1445944 - [mozprofile] Create a new ChromeProfile class for managing chrome profiles

https://reviewboard.mozilla.org/r/236898/#review242770

Awesome
Attachment #8968203 - Flags: review?(rwood) → review+
Comment on attachment 8968204 [details]
Bug 1445944 - [mozbase] Bump mozprofile and mozrunner version numbers

https://reviewboard.mozilla.org/r/236900/#review242774

LGTM
Attachment #8968204 - Flags: review?(rwood) → review+
Blocks: 1454466
Comment on attachment 8968197 [details]
Bug 1445944 - [mozrunner] Remove ability to specify the 'wrap_command' function on an Application context

https://reviewboard.mozilla.org/r/236886/#review242788
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92a5018d1ecf
[moztest] Update shared test fixtures so they can work outside of mozilla-central r=rwood
https://hg.mozilla.org/integration/autoland/rev/eeb6b72619b6
[mozrunner] Remove ability to specify the 'wrap_command' function on an Application context r=rwood
https://hg.mozilla.org/integration/autoland/rev/917baaf08d71
[mozrunner] Convert mozrunner unittests to the pytest format r=davehunt
https://hg.mozilla.org/integration/autoland/rev/df5d226d1df3
[mozrunner] Create a base BlinkRuntimeRunner and add a ChromeRunner to the runners list r=rwood
https://hg.mozilla.org/integration/autoland/rev/74448840c21f
[mozprofile] Convert mozprofile unittests to the pytest format r=davehunt
https://hg.mozilla.org/integration/autoland/rev/96f6bb303871
[mozprofile] Add a 'create_profile' helper method for instanting an instance from an app r=rwood
https://hg.mozilla.org/integration/autoland/rev/62c954751e65
[mozprofile] Pull functionality out of Profile and into an abstract 'BaseProfile' class r=rwood
https://hg.mozilla.org/integration/autoland/rev/fdad1264866f
[mozprofile] Create a new ChromeProfile class for managing chrome profiles r=rwood
https://hg.mozilla.org/integration/autoland/rev/bfabe6333c85
[mozbase] Bump mozprofile and mozrunner version numbers r=rwood
I'll release the new mozrunner/mozprofile versions to pypi once this hits central.
Flags: needinfo?(ahalberstadt)
Released to pypi.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(ahalberstadt)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.