Closed Bug 1358670 Opened 7 years ago Closed 7 years ago

Add Telemetry tests to marionette job in automation

Categories

(Testing :: General, enhancement)

Product:
Testing
Component:
General
Platform:
All
Other
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Silne30, Assigned: Silne30)

References

Details

Attachments

(3 files, 4 obsolete files)

Bug 1358670 - Added requirements and mozharness script
59 bytes, text/x-review-board-request
Details
Bug 1358670 - add telemetry-harness jobs to CI
59 bytes, text/x-review-board-request
Details
Bug 1358670 - Implemented MarionetteHarness in telemetry-harness
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
>>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
Blocks: 1349597
Summary: Add Telemetry tests to marionette job → Add Telemetry tests to marionette job in automation
IS there any other information that I need to provide to get the ball rolling on this?
Flags: needinfo?(dburns)
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 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-
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.
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.
Creating a separate job.
If you are using the MarionetteTestCase then yes. But you have your own harness, and because of that this will not work.
I am working on this.
Status: NEW → ASSIGNED
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)
Assignee: jgriffin → jdorlus
Comment on attachment 8874617 [details]
Bug 1358670 - Added requirements and mozharness script

https://reviewboard.mozilla.org/r/145958/#review151460

Good call. Thanks.
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 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)
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 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.
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.
Attachment #8870594 - Attachment is obsolete: true
Attachment #8874617 - Flags: review?(hskupin)
Attachment #8874617 - Flags: review?(hskupin)
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)
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)
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?
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/
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)
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)
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 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.
Attachment #8878838 - Flags: review?(mshal)
Attachment #8878839 - Flags: review?(dustin)
Attachment #8879679 - Flags: review?(ahalberstadt)
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 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 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 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.
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
Attachment #8879679 - Attachment is obsolete: true
Attachment #8874617 - Flags: review?(hskupin)
Attachment #8878838 - Flags: review?(mshal)
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.
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 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 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 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 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-
Attachment #8880109 - Attachment is obsolete: true
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 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 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?
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 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 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 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.
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.
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.
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.
(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)
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 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 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-
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.
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.
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 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 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 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 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-
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.
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.
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 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?
Attachment #8878839 - Attachment is obsolete: true
Attachment #8878839 - Flags: review?(hskupin)
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 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 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-
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.
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 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.
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.
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.
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 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+
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.
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
Keywords: checkin-needed
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)
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
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
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
Fixed.
Flags: needinfo?(jdorlus)
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.
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)
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?
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.
Agreed that it needs to change before leaving tier 3.
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)
(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)
(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.

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.

Attachment

General

Creator:
Created:
Updated:
Size: