Previous "review" is about feedback on WIP. Thanks.

Comment on attachment 8783904 [details] Bug 1293990 - Add a taskcluster to include session-test in automation; * The taskcluster changes look ok (check them with a try push) but you probably want to set the tier to 3 (See https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy) ** Taskcluster changes should be reviewed by dustin. * If I understand correctly, this WIP copies all the mozharness scripts and configs related to marionette. Instead, I suggest using calling the existing marionette.py mozharness script. You will likely be able to use this script as is (or with minor changes) by providing a session-test-specific config file. * You only need one config file -- your taskcluster task only refers to prod_config.py. Don't duplicate the other configs. ** I might be able to review some of the mozharness changes you need to make, but if it's anything major someone like jlund will have to take a look. Feel free to ask whimboo for feedback as well. He might have more suggestions regarding mozharness as he set up mozharness script for Firefox UI Tests.

Personally I would really feel way more comfortable if we could get all of those tests run under Marionette base with the patch on bug 1296597 landed. Keep in mind that all this extra work here, might be removed again soon.

I would love that too, and I know that's the most elegant and long term solution. But addressing bug 1296597 is far from enough to allow my use case. The "session" branch needed many more changes to its base "marionette" code.

I may need some more guidance on how to declare my taskcluster as a tier 3. Calling marionette.py mozharness script. Duplicated only the prod_config.py config. Still considering it's WIP.

Comment on attachment 8783904 [details] Bug 1293990 - Add a taskcluster to include session-test in automation; https://reviewboard.mozilla.org/r/73570/#review72922 ::: taskcluster/ci/desktop-test/tests.yml:391 (Diff revision 2) > extra-options: > - --reftest-suite=reftest-no-accel > > +session: > + description: "Session unittest run" > + suite: session This suite name doesn't say anything. I think Dustin wants a real name here. What about "Telemetry session test run" and telemetry-session-tests. ::: taskcluster/ci/desktop-test/tests.yml:393 (Diff revision 2) > > +session: > + description: "Session unittest run" > + suite: session > + treeherder-symbol: tc(Ss) > + max-run-time: 5400 Add "tier: 3" here to report as tier-3 level. ::: testing/mozharness/configs/session/prod_config.py:1 (Diff revision 2) > +# This is a template config file for marionette production. This file contains all duplicated code not related to your own tests. As Maja said, strip this down to what you need only. If its only for TC you may not need anything. ::: testing/mozharness/scripts/session.py:1 (Diff revision 2) > +#!/usr/bin/env python Same comment here for naming the script. session.py doesn't say anything.

(In reply to André Reinald from comment #5) > I would love that too, and I know that's the most elegant and long term > solution. But addressing bug 1296597 is far from enough to allow my use > case. The "session" branch needed many more changes to its base "marionette" > code. The question is what has to land in Marionette and what in a specific TelemetryTestCase. I don't think that there are too many changes necessary for Marionette. So if there is something I miss, please comment on bug 1294403.

Comment on attachment 8783904 [details] Bug 1293990 - Add a taskcluster to include session-test in automation; https://reviewboard.mozilla.org/r/73570/#review72922 > This suite name doesn't say anything. I think Dustin wants a real name here. What about "Telemetry session test run" and telemetry-session-tests. "Session" is no less meaningful than and as general as "Marionette". I expect "Session" will be used by other tests than those related to telemetry, so I don't think naming "telemetry" here would be right. > This file contains all duplicated code not related to your own tests. As Maja said, strip this down to what you need only. If its only for TC you may not need anything. I understood Maja told me to drop other config files and keep just this one. I could have a very short "prod_config.py" file including its marionette counterpart instead of duplicating it? > Same comment here for naming the script. session.py doesn't say anything. I didn't intend to make "session" specific to telemetry.

(In reply to Henrik Skupin (:whimboo) from comment #9) > (In reply to André Reinald from comment #5) > > I would love that too, and I know that's the most elegant and long term > > solution. But addressing bug 1296597 is far from enough to allow my use > > case. The "session" branch needed many more changes to its base "marionette" > > code. > > The question is what has to land in Marionette and what in a specific > TelemetryTestCase. I don't think that there are too many changes necessary > for Marionette. So if there is something I miss, please comment on bug > 1294403. To be more specific about the changes from "marionette" to "session": * In the "marionette" branch (harness.marionette) the marionette driver is instantiated (and Firefox launched) from marionette.runner.base.BaseMarionetteTestRunner.run_tests() * In the "session" branch (harness.session) the marionette driver is instantiated from session.session_test.CommonTestCase.setUp() That brought a lot of changes on the way, as the marionette driver instance was used through the "marionette" branch much before tests were started.

Comment on attachment 8783904 [details] Bug 1293990 - Add a taskcluster to include session-test in automation; https://reviewboard.mozilla.org/r/73570/#review73158 ::: testing/mozharness/configs/session/prod_config.py:41 (Diff revision 3) > + ], > + "blob_uploader_auth_file" : os.path.join(os.getcwd(), "oauth.txt"), > + "download_symbols": "ondemand", > + "download_minidump_stackwalk": True, > + "tooltool_cache": "/builds/tooltool_cache", > + "suite_definitions": { All these suite_definitions can go away in your copy of prod_config.py. You will likely define your own suite definition for session instead. ::: testing/mozharness/scripts/session.py:1 (Diff revision 3) > +#!/usr/bin/env python > +# ***** BEGIN LICENSE BLOCK ***** > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, > +# You can obtain one at http://mozilla.org/MPL/2.0/. > +# ***** END LICENSE BLOCK ***** > + > +from marionette import MarionetteTest > + > +if __name__ == '__main__': > + sessionTest = MarionetteTest() > + sessionTest.run_and_exit() MarionetteTest uses marionette's runtests.py. In order to work with your session test harness, your mozharness script needs to define a subclass of MarionetteTest and override `_configure_marionette_virtualenv` to install the correct requirements, as well as override `run_tests` to use your "session" harness. However, it may be worthwhile to revisit the question whether you really do need a separate harness, as whimboo suggests. That would simplify any customization you need in the mozharness script/config.

(In reply to André Reinald from comment #12) > (In reply to Henrik Skupin (:whimboo) from comment #9) > > (In reply to André Reinald from comment #5) > > > I would love that too, and I know that's the most elegant and long term > > > solution. But addressing bug 1296597 is far from enough to allow my use > > > case. The "session" branch needed many more changes to its base "marionette" > > > code. > > > > The question is what has to land in Marionette and what in a specific > > TelemetryTestCase. I don't think that there are too many changes necessary > > for Marionette. So if there is something I miss, please comment on bug > > 1294403. > > To be more specific about the changes from "marionette" to "session": > > * In the "marionette" branch (harness.marionette) the marionette driver is > instantiated (and Firefox launched) from > marionette.runner.base.BaseMarionetteTestRunner.run_tests() > > * In the "session" branch (harness.session) the marionette driver is > instantiated from session.session_test.CommonTestCase.setUp() > > That brought a lot of changes on the way, as the marionette driver instance > was used through the "marionette" branch much before tests were started. Now that you can use quit_in_app (Bug 1296597) to quit and then reset the Marionette session + Firefox instance, it is still a problem that the marionette harness starts a Marionette session before tests begin?

(In reply to Maja Frydrychowicz (:maja_zf) from comment #14) > Now that you can use quit_in_app (Bug 1296597) to quit and then reset the > Marionette session + Firefox instance, it is still a problem that the > marionette harness starts a Marionette session before tests begin? While I can have my tests working, it's a sub-optimal approach as, if a Marionette session is started before my tests begin, I'd first have to quit it (in setUp() I guess), which adds a useless launch-quit cycle to each test. So, if there is an easy way to create a Marionette driver object that doesn't launch Firefox (you told me passing bin=Null to __init__() was ok), and launch Firefox later (and have it connected to the driver), that would be nice!

About how many tests do we actually talk about?

(In reply to Henrik Skupin (:whimboo) from comment #16) > About how many tests do we actually talk about? To my knowledge, bug 1156718 is tracking the tests to be done for telemetry. It points to a spreadsheet listing them: https://docs.google.com/spreadsheets/d/1YxqvjRJuuIPRegNXAFCLHA7_56vhQ6leaZLaLeFqyxY

(In reply to André Reinald from comment #12) > To be more specific about the changes from "marionette" to "session": > > * In the "marionette" branch (harness.marionette) the marionette driver is > instantiated (and Firefox launched) from > marionette.runner.base.BaseMarionetteTestRunner.run_tests() > > * In the "session" branch (harness.session) the marionette driver is > instantiated from session.session_test.CommonTestCase.setUp() > > That brought a lot of changes on the way, as the marionette driver instance > was used through the "marionette" branch much before tests were started. Bringing that together with Maja's tip that it's possible to instantiate a Marionette driver without launching Firefox (by passing the constructor bin=Null), I wonder if we can address my use case inside the "marionette" branch (and dispose of the "session" branch). That would mean adding an option to "mach marionette-test" telling the framework to skip launching Firefox (which my tests would do themselves). Does that make sense? Or would that change break too many things?

So we speak about 28 tests here, whereby some of those might not require Firefox not to get started at the beginning. I cannot say details because the summary of each test is a bit too vague for an outsider. Given that this bug is specifically for a new job addition to taskcluster, I would really move all the general discussion over to bug 1294403.

Comment on attachment 8783904 [details] Bug 1293990 - Add a taskcluster to include session-test in automation; https://reviewboard.mozilla.org/r/73570/#review73158 Still WIP. Trying to fix my mozharness script. > All these suite_definitions can go away in your copy of prod_config.py. You will likely define your own suite definition for session instead. Haven't fixed this yet… to be continued. > MarionetteTest uses marionette's runtests.py. In order to work with your session test harness, your mozharness script needs to define a subclass of MarionetteTest and override `_configure_marionette_virtualenv` to install the correct requirements, as well as override `run_tests` to use your "session" harness. > > However, it may be worthwhile to revisit the question whether you really do need a separate harness, as whimboo suggests. That would simplify any customization you need in the mozharness script/config. I think Henrik made his mind about merging my use case inside the marionette harness for now. Meanwhile, I think that just tweaking the paths to my needs is enough, so I only override query_abs_dirs(). It's unclear exactly what "dirs['abs_marionette_dir']" is, and if I if I changed it correctly. But I understood the "dirs['abs_marionette_tests_dir']" and that one should point to the right place.

Comment on attachment 8783904 [details] Bug 1293990 - Add a taskcluster to include session-test in automation; https://reviewboard.mozilla.org/r/73570/#review73158 > I think Henrik made his mind about merging my use case inside the marionette harness for now. > Meanwhile, I think that just tweaking the paths to my needs is enough, so I only override query_abs_dirs(). > It's unclear exactly what "dirs['abs_marionette_dir']" is, and if I if I changed it correctly. > But I understood the "dirs['abs_marionette_tests_dir']" and that one should point to the right place. The test machine doesn't have a full source checkout, instead it gets a Firefox binary and an archive of relevant tests in target.common.tests.zip. (Look this up in a recent marionette job log on Treeherder to see how the environment is set up: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=marionette&selectedJob=35118033). So the Marionette code and tests are packaged in target.common.tests.zip, which is unpacked on the test machine. `dirs['abs_test_install_dir']` refers to where that archive got unpacked, and within that directory we have: * `dirs['abs_marionette_dir']`, which refers to where the marionette code is (so we can install the python packages; this does NOT include your session-test stuff) * `dirs['abs_marionette_tests_dir']`, which refers to where we look for the test manifest (also does not include your tests right now). So regardless of how you run your tests, you will still need to add your tests to common.tests.zip, assuming you have decided on a good place to store them in-tree. Here is an example of that kind of change: https://hg.mozilla.org/mozilla-central/rev/21a130c2ba15

Comment on attachment 8783904 [details] Bug 1293990 - Add a taskcluster to include session-test in automation; https://reviewboard.mozilla.org/r/73570/#review74084 Please note that I'll be on holiday, returning on Tuesday Sept 6th. You may want to flag whimboo for feedback/review in the mean time. ::: testing/mozharness/scripts/session.py:14 (Diff revision 4) > + def query_abs_dirs(self): > + if self.abs_dirs: > + return self.abs_dirs > + abs_dirs = MarionetteTest.query_abs_dirs() > + dirs['abs_marionette_dir'] = os.path.join( > + dirs['abs_test_install_dir'], 'marionette', 'session') > + dirs['abs_marionette_tests_dir'] = os.path.join( > + dirs['abs_test_install_dir'], 'marionette', 'tests', 'testing', > + 'marionette', 'harness', 'session', 'tests') > + > + for key in dirs.keys(): > + if key not in abs_dirs: > + abs_dirs[key] = dirs[key] > + self.abs_dirs = abs_dirs > + return self.abs_dirs Here you should just update self.abs_dirs with your new values, and return it.

Not sure this will still be needed as we will be using the marionette test harness?

It depends on whether your tests will be added to the manifest used by the Mn job on Treeherder. (https://dxr.mozilla.org/mozilla-central/rev/82d0a583a9a39bf0b0000bccbf6d5c9ec2596bcc/testing/marionette/harness/marionette/tests/unit-tests.ini) If yes (you will need r?AutomatedTester) and this bug is a wontfix. If no... read on. If your tests are to show up as part of some new job symbol on Treeherder, you will need to define a new TaskCluster task, but you most likely won't need to define new mozharness scripts (unless the telemetry tests need some special setup that isn't provided by the marionette.py mozharness script). You may need to add to the mozharness config, or create a new config.

Is there any good reason not to run the tests as part of the Mn job? Otherwise we could just proceed with that and worry about breaking it out later (if there is a need for that).

We should use the Mn job if we can. Andre had some question earlier about whether we could, but my understanding is that now we believe we can use the standard job type.