Closed
Bug 1358670
Opened 8 years ago
Closed 7 years ago
Add Telemetry tests to marionette job in automation
Categories
(Testing :: General, enhancement)
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Silne30, Assigned: Silne30)
References
Details
Attachments
(3 files, 4 obsolete files)
>>Problem:
I am looking to add telemetry tests to the marionette job as they are just another flavor of marionette job. Best case scenario would to be to have these tests show up on treeherder.
>>Solution:
We want tests that can run on checkin of affected components. Hopefully, we could have them sherriffed as well but that needs more discussion.
>>Mozilla Top Level Goal:
Telemetry especially in 57.
>>Existing Bug:
Bug1349597
>>Per-Commit:
It would be nice if it were only run when affected components were touched.
>>Data other than Pass/Fail:
Pass fail is good for now.
>>Prototype Date:
Done
>>Production Date:
ASAP
>>Most Valuable Piece:
The job being constructed with results showing in Treeherder. We already created the custom harness and everything based off of marionette.
>>Responsible Engineer:
jdorlus@mozilla.com
>>Manager:
krupa.mozbugs@gmail.com
>>Other Teams/External Dependencies:
Not for the moment.
>>Additional Info:
:gps gave a good explanation as to what's needed. https://bugzilla.mozilla.org/show_bug.cgi?id=1349597
Assignee | ||
Updated•8 years ago
|
Summary: Add Telemetry tests to marionette job → Add Telemetry tests to marionette job in automation
Assignee | ||
Comment 1•8 years ago
|
||
IS there any other information that I need to provide to get the ball rolling on this?
Flags: needinfo?(dburns)
Comment 2•8 years ago
|
||
Just add your manifest location to https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit-tests.ini and then submit a try run so we can see if they are picked up properly.
Please flag either maja_zf or whimboo for review
Flags: needinfo?(dburns)
Comment hidden (mozreview-request) |
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8870594 [details]
Bug 1358670 - Add telemetry tests to job;
https://reviewboard.mozilla.org/r/142056/#review145882
This is not something which will work. Have you ever tested this locally? What you need is a separate job on Treeherder for your tests.
Attachment #8870594 -
Flags: review?(hskupin) → review-
Comment 5•8 years ago
|
||
To add to my review comment... this is because the telemetry tests have their own testcase class. And this is not known to Marionette proper. Similar to Firefox UI update tests.
Assignee | ||
Comment 6•8 years ago
|
||
I had a feeling that this would be the case but :gps advised me that "A marionette test is a marionette test and there are just different flavors." See comment 1 in 1349597.
Assignee | ||
Comment 7•8 years ago
|
||
Creating a separate job.
Comment 8•8 years ago
|
||
If you are using the MarionetteTestCase then yes. But you have your own harness, and because of that this will not work.
Assignee | ||
Comment 9•7 years ago
|
||
I am working on this.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review151460
Given the constant updates of this patch I do not see that this is already review ready. If feedback is wanted then please ask questions and set the ni? flag.
Also you should split all the changes into individual commits per area. I'm not going to review everything, which I even don't have the permissions for.
Attachment #8874617 -
Flags: review?(hskupin)
Updated•7 years ago
|
Assignee: jgriffin → jdorlus
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review151460
Good call. Thanks.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review151700
Can you please stop requesting review for each change until you are done with the work? This is really polluting my inbox. Thanks. Note, do not add the reviewer name in the comment message, but add it manully on mozreview later.
Attachment #8874617 -
Flags: review?(hskupin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review151838
You promised to stop setting me for review until the work is done, now it happened once again.
Attachment #8874617 -
Flags: review?(hskupin)
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review151838
Apparently, I had fixed this issue in several commits but the the commit message in an old commit still persisted. I have rebased and squashed the commits so that there is only one commit for this bug and made sure the commit message does not include your name. If I need a review, I will do it directly from the command line. I am really sorry about this.
Comment 34•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review151838
As I told you earlier you have to do it in separate commits, best per component to get proper review. Not sure why you reverted this all again.
Assignee | ||
Comment 35•7 years ago
|
||
When this is ready for review, it will be split per component. I needed to rebase to fix the existing issue. Your review will only contain stuff under /testing and I will get someone else for the /taskcluster stuff. Since this isn't working yet anyways, I rebased to get your name out of the commit message once and for all.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8870594 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8874617 -
Flags: review?(hskupin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8874617 -
Flags: review?(hskupin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 45•7 years ago
|
||
Henrik,
I am getting an error in the requirements file about "../telemetry/harness". The directory does exist. I tried the following steps to troubleshoot:
1.) Running testing/config/telemetry_tests_requirements.txt locally. (It runs successfully and installs all deps).
2.) I checked the directory testing/telemetry/harness to make sure that setup.py is there. It is.
3.) The directory structure mimics the firefoxui tests.
I am not sure why this failing. Any insight you can share would be helpful. I used git blame to find out who wrote the firefoxui-tests-requirements file and it was you. So I would like to know if there are any other steps to get the telemetry/harness directory recognized.
Flags: needinfo?(hskupin)
Comment 46•7 years ago
|
||
First, the patch is a big blob of changes which are hard to oversee. I sadly don't have the time to fiddle through all that. But here some answers...
(In reply to John Dorlus [:Silne30] from comment #45)
> 1.) Running testing/config/telemetry_tests_requirements.txt locally. (It
> runs successfully and installs all deps).
Sure, because that's how how wrote the file. But did you test with a generated test archive? That's the place where the requirements file is necessary. Your tests are not running from within a checked out tree.
> 2.) I checked the directory testing/telemetry/harness to make sure that
> setup.py is there. It is.
> 3.) The directory structure mimics the firefoxui tests.
Why are you moving the telemetry harness to /testing? There shouldn't be any need for it. The current location is perfect. See also the external-media-tests which perfectly work that way, and which you want to have a look at.
Keep in mind that firefox-ui tests were placed into /testing because those tests span a wide variety of tests across components. So there is no canonical place they could live otherwise.
Flags: needinfo?(hskupin)
Assignee | ||
Comment 47•7 years ago
|
||
Understood. I can move them back. I did not test with a generated test archive. I was using the task cluster documentation and there is no mention of that anywhere that I have seen. Do you have a link or something that explains that? Is there any doc on this at all?
Comment 48•7 years ago
|
||
You might want to have a look at bug 1212609 for fxui tests. Otherwise you can check the external-media test history, how it has been done for those.
https://dxr.mozilla.org/mozilla-central/source/dom/media/test/external/
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 67•7 years ago
|
||
John, can I ask why you are pushing so many updates and trigger try builds for tests you can perfectly do on your local machine? It's quite an amount of resources you utilize, and not to speak of the time you are loosing by doing it remotely.
Flags: needinfo?(jdorlus)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 70•7 years ago
|
||
Henrik, I have asked around a few times and I was told that pushing to try was the only way to debug the job. I asked the taskcluster team if there was a way to mimic the behavior locally and was told there wasn't a good way. If you know something different, I would gladly do this locally.
Flags: needinfo?(jdorlus)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 74•7 years ago
|
||
I don't know about your problem, but I also don't see why a local execution should not help. So please explain in details what is it about.
Comment 75•7 years ago
|
||
mozreview-review |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review155312
Please do not mix taskcluster, mozharness, and build config changes all in a single patch. You barily find someone who can review that all. Please split this up correctly.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8878838 -
Flags: review?(mshal)
Assignee | ||
Updated•7 years ago
|
Attachment #8878839 -
Flags: review?(dustin)
Attachment #8879679 -
Flags: review?(ahalberstadt)
Comment 80•7 years ago
|
||
mozreview-review |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review155874
::: taskcluster/ci/test/test-sets.yml:53
(Diff revision 2)
> - external-media-tests-base
> - external-media-tests-youtube
>
> +telemetry-tests:
> + - telemetry-tests-client
> +
You've both added this test suite to the `common-tests` test set, and added it to its own test-set. You should do one or the other: if the test is truly meant to be common to all platforms, include it in `common-tests`. Otherwise, remove it from `common-tests` and stick with the self-titled test set you have here.
Attachment #8878839 -
Flags: review?(dustin) → review+
Comment 81•7 years ago
|
||
mozreview-review |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review155888
::: commit-message-dfeee:1
(Diff revision 2)
> +Bug 1358670 - Added test archive for telemetry-tests
> +
> +Added test archive to test-archive.py for test job
I think something went wrong with your diff here, looks like it's adding test_archive.py as a new file.
But I thought you had managed to get it working without a test archive?
Comment 82•7 years ago
|
||
mozreview-review |
Comment on attachment 8879679 [details]
Bug 1358670 - Added requirements and mozharness config
https://reviewboard.mozilla.org/r/151022/#review155890
Nice, this is looking pretty good! Though as we talked about, we want to avoid using the test archive at all.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:119
(Diff revision 1)
> + def download_and_extract(self):
> + """Override method from TestingMixin for more specific behavior."""
> + extract_dirs = ['config/*',
> + 'marionette/*',
> + 'mozbase/*',
> + 'mozharness/',
> + 'telemetry-tests/*'
> + ]
> + super(TelemetryTests, self).download_and_extract(extract_dirs=extract_dirs)
Why do you need to download+extract here if there is a checkout at /home/worker/checkouts/gecko? All the files should be there already, this is doing a bunch of duplicate work and making the tests.zip bigger. Doing this should also mean that you can get rid of the first commit in this series.
You'll need to play around with your dirs and make sure they are set to the proper locations under ~/checkouts/gecko.
Attachment #8879679 -
Flags: review?(ahalberstadt) → review-
Comment 83•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8879679 [details]
Bug 1358670 - Added requirements and mozharness config
https://reviewboard.mozilla.org/r/151022/#review155890
> Why do you need to download+extract here if there is a checkout at /home/worker/checkouts/gecko? All the files should be there already, this is doing a bunch of duplicate work and making the tests.zip bigger. Doing this should also mean that you can get rid of the first commit in this series.
>
> You'll need to play around with your dirs and make sure they are set to the proper locations under ~/checkouts/gecko.
To clarify a bit, I'm talking about stuff like abs_test_install_dir and abs_telemetry_dir. You can probably set them to "~/checkouts/gecko/..." as required.
Assignee | ||
Comment 84•7 years ago
|
||
Going to need to make the following changes to move forward:
1.) Change all references to the test archive to the local source checkout at ~/checkouts/gecko (per TC team)
2.) Remove test_archive.py modifications as we won't be using a test archive
3.) Point all variables in mozharness script to ~/checkouts/gecko rather than test archive
4.) Make sure that the requirements.txt uses test archive.
5.) Leave telemetry-tests in the common-tests section of test-sets.py and remove the telemetry-tests-client listing.
6.) Remove download-extract from mozharness script
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8879679 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8874617 -
Flags: review?(hskupin)
Attachment #8878838 -
Flags: review?(mshal)
Assignee | ||
Comment 88•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review155888
> I think something went wrong with your diff here, looks like it's adding test_archive.py as a new file.
>
> But I thought you had managed to get it working without a test archive?
This has been fixed. Changes to test_archive.py have been removed and also all references to test archives.
Assignee | ||
Comment 89•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review155874
> You've both added this test suite to the `common-tests` test set, and added it to its own test-set. You should do one or the other: if the test is truly meant to be common to all platforms, include it in `common-tests`. Otherwise, remove it from `common-tests` and stick with the self-titled test set you have here.
Removed the self titled test set and left this under common tests. This has been fixed.
Comment 90•7 years ago
|
||
mozreview-review |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review155910
Looks like mozreview got confused here -- this patch was r?ahal
Attachment #8878839 -
Flags: review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 95•7 years ago
|
||
mozreview-review |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review155996
Picture-perfect :)
Attachment #8878838 -
Flags: review?(dustin) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 98•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review156704
Awesome, this is looking good! I think all my remaining comments are pretty minor.
::: testing/config/telemetry_tests_requirements.txt:8
(Diff revision 50)
> +../marionette/client/
> +../marionette/harness/
> +
> +# Allows to use the Puppeteer page object model for Firefox
> +../marionette/puppeteer/firefox/
> +/home/worker/checkouts/gecko/toolkit/components/telemetry/tests/marionette/harness
Make this a relative path too, we shouldn't depend on the checkout being there if we can avoid it:
../../toolkit/...
You'll need to use the copy found in the source for this (see later comment).
::: testing/mozharness/scripts/telemetry/telemetry_client.py:16
(Diff revision 50)
> +import sys
> +
> +# load modules from parent dir
> +sys.path.insert(1, os.path.dirname(os.path.dirname(sys.path[0])))
> +
> +TELEMETRY_TEST_HOME = "/home/worker/checkouts/gecko/toolkit/components/telemetry/tests/marionette/"
A few things here:
1. Use '~' with os.path.expanduser, so you we don't need to hardcode /home/worker.
2. Use os.path.join() instead of '/'.
3. Create an intermediate variable called GECKO_SRCDIR. You'll be able to re-use it down below for the requirements.txt.
4. I filed bug 1375496. Please add a comment referencing that so we can update it to not hardcode the gecko location (once that bug is fixed).
Thanks!
::: testing/mozharness/scripts/telemetry/telemetry_client.py:57
(Diff revision 50)
> + 'dest': 'e10s',
> + 'action': 'store_true',
> + 'default': False,
> + 'help': 'Enable multi-process (e10s) mode when running tests.',
> + }],
> + [['--symbols-path=SYMBOLS_PATH'], {
I think while this will technically work, specifying `--foo=BAR` as the option string isn't supported by argparse. Under the hood, argparse thinks the option works like `--foo=BAR=baz`. If you want BAR to show up in the help, use the metavar key.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:108
(Diff revision 50)
> + requirements = os.path.join(dirs['abs_test_install_dir'],
> + 'config', 'telemetry_tests_requirements.txt')
Use the telemetry_tests_requirements.txt from the source checkout.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:170
(Diff revision 50)
> + # Resource files to serve via local webserver
> + '--server-root', os.path.join(dirs['abs_telemetry_dir'], 'harness', 'www'),
> +
> +
> + # Use the work dir to get temporary data stored
> + '--workspace', dirs['abs_work_dir'],
I'd recommend using tempfile for this, or at least creating a subdirectory within abs_work_dir.
Attachment #8874617 -
Flags: review?(ahalberstadt)
Comment 99•7 years ago
|
||
mozreview-review |
Comment on attachment 8880109 [details]
Bug 1358670 - Removed another reference to abs_test_install_dir
https://reviewboard.mozilla.org/r/151486/#review156740
Ah, I see you fixed one of my comments from the previous commit here. Please roll this change into the earlier commit and discard this one.
Attachment #8880109 -
Flags: review?(ahalberstadt) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8880109 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 108•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review157144
Thanks looks good! R+ with the two comments addressed.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:18
(Diff revision 52)
> +TELEMETRY_TEST_HOME = os.path.join(os.path.expanduser('~'),
> + GECKO_SRCDIR, 'toolkit', 'components', 'telemetry'
> + 'tests', 'marionette')
This will resolve to /home/worker/home/worker/.. Don't do expanduser the second time.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:68
(Diff revision 52)
> + 'dest': 'symbols_path',
> + 'metavar': 'SYMBOLS_PATH',
> + 'help': 'absolute path to directory containing breakpad '
> + 'symbols, or the url of a zip file containing symbols.',
> + }],
> + [['--tag=TAG'], {
Remove the =TAG here as well.
Attachment #8874617 -
Flags: review?(ahalberstadt) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 112•7 years ago
|
||
mozreview-review |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review157254
::: commit-message-dfeee:1
(Diff revision 9)
> + Bug 1358670 - Taskcluster configuration
It's a minor thing, but I just noticed -- this should be more descriptive.
Bug 1358670 - add telemetry-harness jobs to CI
The idea here is that the reader of that commit message doesn't have the context that this was part of work on telemetry-harness.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 118•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review156704
> I'd recommend using tempfile for this, or at least creating a subdirectory within abs_work_dir.
I really am not sure what this variable references or how it affects the context of the running tests. This command line arg is in all of the test suites I have seen. Do you know what this arg actually does?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 125•7 years ago
|
||
This is working on taskcluster. The tests successfully ran and it uses no test archive. The tests failed but that has to do with debugging the actual test cases, not the job itself. Once Henrik gives his review, we should be able to land this.
Flags: needinfo?(hskupin)
Comment 126•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review157882
::: testing/mozharness/scripts/telemetry/telemetry_client.py:68
(Diff revision 56)
> +
> +
> +class TelemetryTests(TestingMixin, VCSToolsScript, CodeCoverageMixin):
> +
> + # Needs to be overwritten in sub classes
> + cli_script = None
Looks like you only have a single entry script. So you can drop this.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:81
(Diff revision 56)
> + 'download-and-extract',
> + 'create-virtualenv',
> + 'install',
> + 'run-tests',
> + 'uninstall',
> + ]
Make sure that you only list those steps which are really in use.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:126
(Diff revision 56)
> + abs_dirs[key] = dirs[key]
> + self.abs_dirs = abs_dirs
> +
> + return self.abs_dirs
> +
> + def query_harness_args(self, extra_harness_config_options=None):
Please check if you really need this. It's used in Fxui tests because we have differnt entry scripts.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:163
(Diff revision 56)
> + '--address', 'localhost:{}'.format(marionette_port),
> +
> + # Resource files to serve via local webserver
> + '--server-root', os.path.join(dirs['abs_telemetry_dir'], 'harness', 'www'),
> +
> +
nit: Remove the extra empty line.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:235
(Diff revision 56)
> + os.path.join('client', 'manifest.ini'),
> + os.path.join('unit', 'manifest.ini'),
> + ]
> +
> +
> +class TelemetryServerTests(TelemetryTests):
I don't see that this class is used at all. Are you going to run this somewhere? Did you miss to add it?
Attachment #8874617 -
Flags: review-
Comment 127•7 years ago
|
||
mozreview-review |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review157900
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:35
(Diff revision 13)
>
> # Start and configure server
> - self.httpd = httpd.FixtureServer(doc_root)
> + # if not args.server_root:
> + # self.server_root = doc_root
> + # TODO: Bug 1373993 - Make this accept an argument from the command line.
> + self.httpd = httpd.FixtureServer(
Is the goal to only run the tests on Linux? I ask because on other platforms this will totally break.
::: toolkit/components/telemetry/tests/marionette/tests/client/manifest.ini:4
(Diff revision 13)
> [DEFAULT]
> tags = client
>
> -[test_main_ping_addon_install_tab_window_scalars.py]
> +[test_main_tab_scalars.py]
I don't see that you are renaming the test file.
Attachment #8878839 -
Flags: review?(hskupin) → review-
Comment 128•7 years ago
|
||
mozreview-review |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review157902
::: testing/mozharness/scripts/telemetry/telemetry_client.py:150
(Diff revision 13)
>
> def run_test(self, binary_path, env=None, marionette_port=2828):
> """All required steps for running the tests against an installer."""
> dirs = self.query_abs_dirs()
>
> + print "the dirs are: " + str(dirs)
Is that a left-over from debugging? Please remove it.
Assignee | ||
Comment 129•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review157900
> Is the goal to only run the tests on Linux? I ask because on other platforms this will totally break.
Since yhere is no real difference between telemetry in Linux and Windows, it doesn't seem that there is a need to run the tests on both platforms. So these were only going to run on Linux.
Assignee | ||
Comment 130•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review157882
> Make sure that you only list those steps which are really in use.
All of the steps are in use. Download-and-extract, install and uninstall are needed to have a firefox instance to run our tests against. Appartently this will change soon but hasn't yet.
> Please check if you really need this. It's used in Fxui tests because we have differnt entry scripts.
Removed and also removed the reference to it.
> I don't see that this class is used at all. Are you going to run this somewhere? Did you miss to add it?
This was done in preparation for some server tests that we will be getting up and running. I will remove it and add it when it's needed.
Assignee | ||
Comment 131•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review157900
> I don't see that you are renaming the test file.
This test was previously renamed. The job had failed because the manifest file wasn't updated at the time. So I renamed the test in the manifest file to address the error.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 135•7 years ago
|
||
(In reply to John Dorlus [:Silne30] from comment #129)
> > Is the goal to only run the tests on Linux? I ask because on other platforms this will totally break.
>
> Since yhere is no real difference between telemetry in Linux and Windows, it
> doesn't seem that there is a need to run the tests on both platforms. So
> these were only going to run on Linux.
Georg, can you please give your feedback here too? I find it strange that we only run this on a single platform. If you want to extend later, it's fine.
Flags: needinfo?(hskupin) → needinfo?(gfritzsche)
Comment 136•7 years ago
|
||
Telemetry tests need to run across platforms.
The assumption that there are no real differences for Telemetry between platforms is incorrect.
Flags: needinfo?(gfritzsche)
Comment 137•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review157900
> Since yhere is no real difference between telemetry in Linux and Windows, it doesn't seem that there is a need to run the tests on both platforms. So these were only going to run on Linux.
As Georg said tests should be run on all platforms. Please clarify if that should happen immediately or as a follow-up. Until then this issue should not have been resolved.
> This test was previously renamed. The job had failed because the manifest file wasn't updated at the time. So I renamed the test in the manifest file to address the error.
You should describe those things and ideally make this a separate commit.
Comment 138•7 years ago
|
||
mozreview-review |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review158356
::: commit-message-5fb43:3
(Diff revision 14)
> +Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
> +
> +MozReview-Commit-ID: KBKhWw7DrAw
Ok, so this needs a more detailed explanation what is done here.
::: toolkit/components/telemetry/tests/marionette/harness/setup.py:9
(Diff revision 14)
>
> import os
>
> from setuptools import setup, find_packages
>
> -PACKAGE_VERSION = '0.1'
> +PACKAGE_VERSION = '0.3'
Any reason why this goes from 0.1 to 0.3?
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:36
(Diff revision 14)
> # Start and configure server
> - self.httpd = httpd.FixtureServer(doc_root)
> + # if not args.server_root:
> + # self.server_root = doc_root
> + # TODO: Bug 1373993 - Make this accept an argument from the command line.
> + self.httpd = httpd.FixtureServer(
> + '/home/worker/checkouts/gecko/toolkit/components/telemetry/tests/marionette/harness/www')
If we need multiple platforms immediately this has to be fixed now.
Attachment #8878839 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 139•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review157900
> As Georg said tests should be run on all platforms. Please clarify if that should happen immediately or as a follow-up. Until then this issue should not have been resolved.
Yes. This will be fixed.
Assignee | ||
Comment 140•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review157900
> You should describe those things and ideally make this a separate commit.
Will do.
Assignee | ||
Comment 141•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review158356
> Any reason why this goes from 0.1 to 0.3?
This must have been a typo. I will adjust it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 148•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review159518
::: testing/mozharness/scripts/telemetry/telemetry_client.py:36
(Diff revisions 56 - 57)
> from mozharness.mozilla.vcstools import VCSToolsScript
>
> -
> # General command line arguments for Firefox ui tests
> telemetry_tests_config_options = [
> - [["--allow-software-gl-layers"], {
> + [["--allow-software-gl-layers"], {
Can you explain why you did all this strange reformatting of code? Please revert that to what it was before. This indentation is wrong.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:194
(Diff revisions 56 - 57)
> )
>
>
> class TelemetryClientTests(TelemetryTests):
> -
> cli_script = 'runtests.py'
Remove this too.
Attachment #8874617 -
Flags: review-
Comment 149•7 years ago
|
||
mozreview-review |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review159520
::: taskcluster/ci/test/tests.yml:1486
(Diff revision 14)
> - --webServer,localhost
>
> +telemetry-tests-client:
> + description: "Telemetry tests client run"
> + suite: telemetry-tests-client
> + treeherder-symbol: tc-tt-c
Just curious. Why do we need the `tc-` prefix here? I would suggest that reports go directly into the `tc-e10s` group.
We had to use a separate group for firefox ui tests because we also have mozmill-ci reporting to Treeherder.
::: taskcluster/ci/test/tests.yml:1495
(Diff revision 14)
> + tier: 2
> + docker-image: {"in-tree": "desktop1604-test"}
> + mozharness:
> + script: telemetry/telemetry_client.py
> + config:
> + - remove_executables.py
For cross-platform support you will have to only use it for Linux. It will break any config on Windows and OS X.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:200
(Diff revision 14)
> os.path.join('client', 'manifest.ini'),
> os.path.join('unit', 'manifest.ini'),
> ]
>
> -
> if __name__ == '__main__':
The linter will not fail but I would leave the 2 empty rows here to separate the global code from the class definition.
Comment 150•7 years ago
|
||
mozreview-review |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review159522
Make this part of the former commit. Otherwise it will be broken.
Attachment #8882170 -
Flags: review?(hskupin) → review-
Comment 151•7 years ago
|
||
mozreview-review |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review159524
::: commit-message-5fb43:5
(Diff revisions 14 - 17)
> Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
>
> +Added changes to testcase.py for change server root
> +Added argument.py and runner.py to implement MarionetteHarness in the test job
> +
As someone who is not familar with the code, the summary and description is not helpful to get an understanding that this patch is actually trying to accomplish. Please fix both with better wording.
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:22
(Diff revisions 14 - 17)
> from marionette_harness.runner import httpd
>
> -here = os.path.abspath(os.path.dirname(__file__))
> -doc_root = os.path.join(os.path.dirname(here), "www")
> -resources_dir = os.path.join(os.path.dirname(here), "resources")
> +# TODO: Bug 1373993 - Make this accept an argument from the command line.
> +GECKO_SRCDIR = os.path.join(os.path.expanduser('~'), 'checkouts', 'gecko')
> +TELEMETRY_TEST_HOME = os.path.join(GECKO_SRCDIR, 'toolkit', 'components', 'telemetry',
> + 'tests', 'marionette')
All the paths here only apply to the mozharness script when it gets run under automation. This code should not end-up in the custom Marionette harness runner.
Lets just use the "--server-root" argument which your harness gets for free from the Marionette harness.
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:37
(Diff revisions 14 - 17)
> super(TelemetryTestCase, self).setUp()
>
> - # Start and configure server
> - # if not args.server_root:
> - # self.server_root = doc_root
> # TODO: Bug 1373993 - Make this accept an argument from the command line.
The todo doesn't apply here anymore and can be removed.
Attachment #8878839 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 152•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review159518
> Can you explain why you did all this strange reformatting of code? Please revert that to what it was before. This indentation is wrong.
Interesting. Not sure why that happened as I don't bother these args. Fixed.
Assignee | ||
Comment 153•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878838 [details]
Bug 1358670 - add telemetry-harness jobs to CI
https://reviewboard.mozilla.org/r/150096/#review159520
> The linter will not fail but I would leave the 2 empty rows here to separate the global code from the class definition.
Fixed.
Assignee | ||
Comment 154•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878839 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/150098/#review159524
> All the paths here only apply to the mozharness script when it gets run under automation. This code should not end-up in the custom Marionette harness runner.
>
> Lets just use the "--server-root" argument which your harness gets for free from the Marionette harness.
Used a testvar to retrieve the set server_root from the harness.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 166•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review159522
Is this not the same change that you told me to separate into its own commit?
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8878839 -
Attachment is obsolete: true
Attachment #8878839 -
Flags: review?(hskupin)
Comment 169•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review159522
Sure, and my previous comment wasn't correct. It should have been 'Put it before the former commit...'. If you add the commit as last one, you will introduce a broken test suite for the commit before.
Comment 170•7 years ago
|
||
mozreview-review |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review162134
::: testing/config/telemetry_tests_requirements.txt:8
(Diff revision 59)
> +../marionette/client/
> +../marionette/harness/
> +
> +# Allows to use the Puppeteer page object model for Firefox
> +../marionette/puppeteer/firefox/
> +../../toolkit/components/telemetry/tests/marionette/harness
nit: best add a new line above otherwise it looks like that telemetry is related to puppeteer.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:19
(Diff revision 59)
> +sys.path.insert(1, os.path.dirname(os.path.dirname(sys.path[0])))
> +
> +GECKO_SRCDIR = os.path.join(os.path.expanduser('~'), 'checkouts', 'gecko')
> +
> +TELEMETRY_TEST_HOME = os.path.join(GECKO_SRCDIR, 'toolkit', 'components', 'telemetry',
> + 'tests', 'marionette')
I don't see a need for defining those global constants, but if it makes you happy leave them here.
::: testing/mozharness/scripts/telemetry/telemetry_client.py:21
(Diff revision 59)
> +GECKO_SRCDIR = os.path.join(os.path.expanduser('~'), 'checkouts', 'gecko')
> +
> +TELEMETRY_TEST_HOME = os.path.join(GECKO_SRCDIR, 'toolkit', 'components', 'telemetry',
> + 'tests', 'marionette')
> +
> +from mozharness.base.log import FATAL, WARNING
Is WARNING and FATAL used somewhere?
::: testing/mozharness/scripts/telemetry/telemetry_client.py:67
(Diff revision 59)
> + 'symbols, or the url of a zip file containing symbols.',
> + }],
> + [['--tag=TAG'], {
> + 'dest': 'tag',
> + 'help': 'Subset of tests to run (local, remote).',
> + }],
Why has this been added with the last revisions? It would be great if you can focus on the open issues, and not introduce new code, which is even not used at all.
Attachment #8874617 -
Flags: review-
Comment 171•7 years ago
|
||
mozreview-review |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review162176
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/argument.py:18
(Diff revision 7)
> +
> +class TelemetryArguments(BaseMarionetteArguments):
> +
> + def __init__(self, **kwargs):
> + super(TelemetryArguments, self).__init__(**kwargs)
> + self.register_argument_container(TelemetryBaseArguments())
This is a no-op. Why did you add this file at all?
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:26
(Diff revision 7)
>
> ping_list = []
>
> - def setUp(self, *args, **kwargs):
> - super(TelemetryTestCase, self).setUp()
> + def __init__(self, *args, **kwargs):
> + super(TelemetryTestCase, self).__init__(*args, **kwargs)
> + self.logger.info(kwargs)
This is debug, which you want to remove.
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:30
(Diff revision 7)
> - super(TelemetryTestCase, self).setUp()
> + super(TelemetryTestCase, self).__init__(*args, **kwargs)
> + self.logger.info(kwargs)
>
> - # Start and configure server
> - self.httpd = httpd.FixtureServer(doc_root)
> + def setUp(self, *args, **kwargs):
> + super(TelemetryTestCase, self).setUp(*args, **kwargs)
> + self.logger.info(self.testvars)
Same here.
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:53
(Diff revision 7)
>
> # Firefox will be forced to restart with the prefs enforced.
> self.marionette.enforce_gecko_prefs(telemetry_prefs)
>
> def wait_for_ping(self):
> if len(self.ping_list) == 0:
As you asked on IRC the ping_list never gets reset between tests.
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:55
(Diff revision 7)
> self.marionette.enforce_gecko_prefs(telemetry_prefs)
>
> def wait_for_ping(self):
> if len(self.ping_list) == 0:
> try:
> Wait(self.marionette, 60).until(lambda t: len(self.ping_list) > 0)
If you don't need `t` just replace it with `_`.
Attachment #8882170 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 172•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review162176
> As you asked on IRC the ping_list never gets reset between tests.
https://bugzilla.mozilla.org/show_bug.cgi?id=1380748 Filed a bug for this. Since this bug is just to get the task cluster job working, I will fix that issue in the aforementioned bug.
Assignee | ||
Comment 173•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review162176
> This is a no-op. Why did you add this file at all?
I thought that this was the way I could pass args from harness to testcase. Removing.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 177•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review162176
> I thought that this was the way I could pass args from harness to testcase. Removing.
You are not adding any specific argument for your harness here. So there absolutely no behavior change, and as such calling those methods is useless. You would need it once you require your own arguments.
Assignee | ||
Comment 178•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review162134
> Why has this been added with the last revisions? It would be great if you can focus on the open issues, and not introduce new code, which is even not used at all.
What are you referring to here? These args were added since the beginning. I had to fix a code reformat. This is not new code. Now, in terms of the code not being used, it's hard to tell what's needed and what's isn't. I still haven't seen any docs for the mozharness scripts so other script examples have been my reference. Every other mozharness script I saw had at least these args (some had more). So I followed suit.
Assignee | ||
Comment 179•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review162134
> Is WARNING and FATAL used somewhere?
No. Removed.
Assignee | ||
Comment 180•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script
https://reviewboard.mozilla.org/r/145958/#review162134
> I don't see a need for defining those global constants, but if it makes you happy leave them here.
I think I will leave them. Looks cleaner to me.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 184•7 years ago
|
||
mozreview-review |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review164040
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:52
(Diff revision 9)
> self.marionette.enforce_gecko_prefs(telemetry_prefs)
>
> - def wait_for_ping(self):
> + def wait_for_ping(self, current_ping_list_size):
> - if len(self.ping_list) == 0:
> try:
> - Wait(self.marionette, 60).until(lambda t: len(self.ping_list) > 0)
> + Wait(self.marionette, 60).until(lambda _: len(self.ping_list) > current_ping_list_size)
With that change you are moving out the internal logic to the test itself, which has to keep track of the ping list entries.
As I have mentioned a long time ago, you should better use a callback argument here so that you have something like:
```
def send_ping(self, callback):
cur_pings = len(self.ping_list)
callback()
Wait().until(len(self.ping_list > cur_pings)
```
I'm fine with doing that in a follow-up. But please file it as a new bug before marking this issue as resolved.
::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:54
(Diff revision 9)
> - def wait_for_ping(self):
> + def wait_for_ping(self, current_ping_list_size):
> - if len(self.ping_list) == 0:
> try:
> - Wait(self.marionette, 60).until(lambda t: len(self.ping_list) > 0)
> + Wait(self.marionette, 60).until(lambda _: len(self.ping_list) > current_ping_list_size)
> except Exception as e:
> self.fail('Error generating ping: {}'.format(e.message))
Why can't the error bubble up as usual? You are hiding the real underlying error message here. Please also file a new bug for it.
Attachment #8882170 -
Flags: review?(hskupin) → review+
Assignee | ||
Comment 185•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review164040
> With that change you are moving out the internal logic to the test itself, which has to keep track of the ping list entries.
>
> As I have mentioned a long time ago, you should better use a callback argument here so that you have something like:
>
> ```
> def send_ping(self, callback):
> cur_pings = len(self.ping_list)
> callback()
> Wait().until(len(self.ping_list > cur_pings)
> ```
>
> I'm fine with doing that in a follow-up. But please file it as a new bug before marking this issue as resolved.
https://bugzilla.mozilla.org/show_bug.cgi?id=1382345
Thanks for the suggestion again. Made sure to record that.
Assignee | ||
Comment 186•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882170 [details]
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
https://reviewboard.mozilla.org/r/153284/#review164040
> Why can't the error bubble up as usual? You are hiding the real underlying error message here. Please also file a new bug for it.
https://bugzilla.mozilla.org/show_bug.cgi?id=1382346
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 187•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s 01ebe68882fd -d 4a7dd8750616: rebasing 408044:01ebe68882fd "Bug 1358670 - Added requirements and mozharness script r=ahal"
rebasing 408045:e19377463a1c "Bug 1358670 - add telemetry-harness jobs to CI r=dustin"
merging taskcluster/ci/test/test-sets.yml
merging taskcluster/ci/test/tests.yml
merging taskcluster/taskgraph/transforms/task.py
rebasing 408046:81c0ce4a58bb "Bug 1358670 - Implemented MarionetteHarness in telemetry-harness r=whimboo" (tip)
merging toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py
merging toolkit/components/telemetry/tests/marionette/tests/client/test_main_tab_scalars.py
warning: conflicts while merging toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/telemetry/tests/marionette/tests/client/test_main_tab_scalars.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 191•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0c5cf70baa50
Added requirements and mozharness script r=ahal
https://hg.mozilla.org/integration/autoland/rev/e04b9716ebad
add telemetry-harness jobs to CI r=dustin
https://hg.mozilla.org/integration/autoland/rev/ab3693949c72
Implemented MarionetteHarness in telemetry-harness r=whimboo
Backed out for flake8 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=116069821&repo=autoland
https://hg.mozilla.org/integration/autoland/rev/a0a736fac5d50435a72ca133de2e1c9e2111972c
Flags: needinfo?(jdorlus)
Comment 193•7 years ago
|
||
The new tc-e10s failed:
[task 2017-07-20T18:32:36.252540Z] 18:32:36 INFO - Calling ['/home/worker/workspace/build/venv/bin/python', '/home/worker/workspace/build/venv/lib/python2.7/site-packages/telemetry_harness/runtests.py', '--binary', '/home/worker/workspace/build/application/firefox/firefox', '--address', 'localhost:2828', '--server-root', '/home/worker/checkouts/gecko/toolkit/components/telemetry/tests/marionette/harness/www', '--workspace', '/home/worker/workspace/build', '--gecko-log=-', '--log-raw=-', '--log-html', '/home/worker/workspace/build/blobber_upload_dir/report.html', '--log-xunit', '/home/worker/workspace/build/blobber_upload_dir/report.xml', '-vv', '/home/worker/checkouts/gecko/toolkit/components/telemetry/tests/marionette/tests/client/manifest.ini', '/home/worker/checkouts/gecko/toolkit/components/telemetry/tests/marionette/tests/unit/manifest.ini'] with output_timeout 300
[task 2017-07-20T18:32:36.538125Z] 18:32:36 INFO - Traceback (most recent call last):
[task 2017-07-20T18:32:36.539120Z] 18:32:36 INFO - File "/home/worker/workspace/build/venv/lib/python2.7/site-packages/telemetry_harness/runtests.py", line 7, in <module>
[task 2017-07-20T18:32:36.539925Z] 18:32:36 INFO - from argument import TelemetryArguments
[task 2017-07-20T18:32:36.540706Z] 18:32:36 INFO - ImportError: No module named argument
[task 2017-07-20T18:32:36.569702Z] 18:32:36 ERROR - Return code: 1
https://treeherder.mozilla.org/logviewer.html#?job_id=116074567&repo=autoland
Comment hidden (mozreview-request) |
Comment 195•7 years ago
|
||
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d80cfbe62d1
Added requirements and mozharness script r=ahal
https://hg.mozilla.org/integration/autoland/rev/d9ab99de4263
add telemetry-harness jobs to CI r=dustin
https://hg.mozilla.org/integration/autoland/rev/32492446c1bd
Implemented MarionetteHarness in telemetry-harness r=whimboo
Comment 197•7 years ago
|
||
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cff5c3f0af9b
Turn these jobs to tier-3 until they get cleaned up a=bustage CLOSED TREE
These jobs were failing like https://treeherder.mozilla.org/logviewer.html#?job_id=116146237&repo=autoland so I flipped them to tier-3 so they're out of the main Treeherder view until they get cleaned up. Once they're cleaned up, you can just undo that commit and flip them back to tier-2.
Comment 199•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d80cfbe62d1
https://hg.mozilla.org/mozilla-central/rev/d9ab99de4263
https://hg.mozilla.org/mozilla-central/rev/32492446c1bd
https://hg.mozilla.org/mozilla-central/rev/cff5c3f0af9b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 200•7 years ago
|
||
This has to be backed out immediately because of the very poor choice of job symbol (tc-e10s) it is hiding all the Mn jobs which should be under the tc-e10s GROUP.
CC'ing sheriffs for that action. So whoever comes first...
Flags: needinfo?(wkocher)
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)
Comment 201•7 years ago
|
||
Hm, or not? The Treeherder link Wes posted looks very suspicious:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=32492446c1bdba6278479ca3fb7f723d9483b081&selectedJob=116146237
But when I check autoland now, it looks fine? Do I miss something?
Comment 202•7 years ago
|
||
Anyway, the wording of the job name should never have been 'tc-e10s'. At least not at this point, and as long the 'tc-e10s' group exists. The sheriffs can decide what's best.
The jobs are tier-3, so you'll need to opt-in to seeing tier-3 jobs: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=4bd34877b405a9417791dc975af195ad32507739&fromchange=32492446c1bdba6278479ca3fb7f723d9483b081&tochange=83363abc4db1a7a5ca44896f8a7cd2077efdb748&filter-tier=1&filter-tier=2&filter-tier=3
As tier-3 jobs, I don't think they need backed out. Would be nice to get a more descriptive job symbol before bringing them back to tier-2 or -1, though.
Flags: needinfo?(wkocher)
"Tt" or "tc-e10s(Tt)" (to group these with the Mn jobs?) sound like good alternative symbols, fwiw.
Comment 205•7 years ago
|
||
Agreed that it needs to change before leaving tier 3.
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)
Comment 206•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #202)
> Anyway, the wording of the job name should never have been 'tc-e10s'. At
> least not at this point, and as long the 'tc-e10s' group exists. The
> sheriffs can decide what's best.
Richard, can you please make sure to get this done before we move to tier 2 or 1? Thanks.
Flags: needinfo?(rpappalardo)
Comment 207•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #206)
> (In reply to Henrik Skupin (:whimboo) from comment #202)
> > Anyway, the wording of the job name should never have been 'tc-e10s'. At
> > least not at this point, and as long the 'tc-e10s' group exists. The
> > sheriffs can decide what's best.
>
> Richard, can you please make sure to get this done before we move to tier 2
> or 1? Thanks.
Thank you, Henrik. I'll take a look, but I'm not sure who's going to pickup John's work atm, so this may sit open for awhile until we've figured that out.
Comment 208•6 years ago
|
||
Per :raphael, these tests are running now on tier 2 under tt(c):
https://searchfox.org/mozilla-central/source/taskcluster/ci/test/misc.yml#56
so, closing
Flags: needinfo?(rpappalardo)
You need to log in
before you can comment on or make changes to this bug.
Description
•