Closed Bug 1201588 Opened 9 years ago Closed 9 years ago

Refactor crash symbols handling for firefox-ui-tests scripts

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

As seen on bug 1200368 the handling of crash symbols is not quite great. We have to do a refactoring here to allow maybe the following:

* Have a Symbols mixin class which can be used by different mozharness scripts without having to include the TestingMixin

* For update tests we should not try to magically find symbols for a build number of the target build, but use the source build instead. Not sure where to grab the symbols from yet but there should be a way I'm sure.
Also we should not download the symbols immediately but do it ondemand. That saves traffic and a couple seconds depending on your network connection.
Armen, I would tend to use mozdownload to actually get the URL of the crash symbols if no URL gets set when calling the mozharness script. With the upcoming release of mozdownload this will become kinda simple. Here an example for a Nightly build from 5th September for win32:

>>> scraper = factory.FactoryScraper('daily', platform='win32', build_id='20150905030205', extension='crashreporter-symbols.zip')
 INFO | Retrieving list of builds from https://archive.mozilla.org/pub/mozilla.org/firefox/nightly/2015/09
 INFO | Found 1 build: 2015-09-05-03-02-05-mozilla-central
 INFO | Selected build: 2015-09-05-03-02-05-mozilla-central
>>> scraper.final_url
u'https://archive.mozilla.org/pub/mozilla.org/firefox/nightly/2015/09/2015-09-05-03-02-05-mozilla-central/firefox-43.0a1.en-US.win32.crashreporter-symbols.zip'

What do you think? Would also the release tests benefit from it? Or should I still allow this for our test scripts only?
Flags: needinfo?(armenzg)
Here actually an example of a candidate build:

>>> scraper = factory.FactoryScraper('candidate', platform='win32', version='41.0b6', build_number='1', extension='crashreporter-symbols.zip')
 INFO | Retrieving list of candidate builds from https://archive.mozilla.org/pub/mozilla.org/firefox/candidates/41.0b6-candidates/
 INFO | Found 1 build: build1
 INFO | Selected build: build1
>>> scraper.final_url
u'https://archive.mozilla.org/pub/mozilla.org/firefox/candidates/41.0b6-candidates/build1/win32/en-US/firefox-41.0b6.crashreporter-symbols.zip'
I think it would work, however, I already have the code. It would mainly be a matter of refactoring.
Flags: needinfo?(armenzg)
So I talked to Ted on IRC and he had some great news for us regarding fetching the crash symbols. Since a couple of days there is no need anymore to pass in the url of the symbols to mozcrash. It's enough to set the local symbols path (empty dir) and the minidump stackwalk binaries in the tooltool repository will download the symbols automatically. 

Here the example for Linux:
https://dxr.mozilla.org/mozilla-central/source/testing/config/tooltool-manifests/linux32/releng.manifest

So as I have seen there is a TooltoolMixin class available in mozharness which I will check if it can be used for our purposes. I hope it will also work with the developer mode.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
You can pull them from tooltool in mozharness if you need to, or you can just grab the binaries from tooltool manually and put them somewhere (and set MINIDUMP_STACKWALK=/path/to/minidump_stackwalk), they don't change that often.
Ted, the TestingMixin has a couple of helper methods for fetching the correct stackwalk binary depending on the platform. I could duplicate those methods or let our tests base on the TestingMixin. Not sure if the latter makes sense given that we are not in mozilla-central and aren't getting run via buildbot. 

Maybe we can make those helper methods available through a TooltoolTestingMixin or such?
Flags: needinfo?(ted)
I'm really not a mozharness expert, sorry.
Flags: needinfo?(ted)
Depends on: 1208184
Depends on: 1208431
Attached patch crash symbols v1Splinter Review
Close to be finished patch. It only contains the additional changes which need to be landed in the blocking bug for testbase.py. So for now I would like to see what you think about the changes, Armen.
Attachment #8665977 - Flags: feedback?(armenzg)
Comment on attachment 8665977 [details] [diff] [review]
crash symbols v1

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

::: testing/mozharness/configs/firefox_ui_tests/qa_jenkins.py
@@ +9,5 @@
> +    },
> +
> +    # General local variable overwrite
> +    'exes': {
> +        'gittool.py': os.path.join(os.getcwd(), 'external_tools', 'gittool.py'),

Not sure if there is a better way as using getcwd(). It will fail if the script is not called from mozharness root.

::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ +106,5 @@
> +        super(FirefoxUITests, self).__init__(
> +            config_options=config_options,
> +            all_actions=all_actions or actions,
> +            default_actions=default_actions or actions,
> +            *args, **kwargs)

I reverted that so we are in sync with other mozharness scripts. Also the current way doesn't seem to work very well.
Comment on attachment 8665977 [details] [diff] [review]
crash symbols v1

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

This lgtm.

::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ +45,4 @@
>          'help': 'absolute path to directory containing breakpad '
>                  'symbols, or the url of a zip file containing symbols.',
>      }],
> +] + copy.deepcopy(testing_config_options)

This adds a bunch of other parameters we're likely to not care.
Are you sure you want all of these?

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/testbase.py#36

@@ +192,5 @@
> +        manifest_path = None
> +
> +        if self.config.get('download_minidump_stackwalk'):
> +            tooltool_manifest = self.query_minidump_tooltool_manifest()
> +            url_base = 'https://hg.mozilla.org/mozilla-central/raw-file/default/testing/'

This will point us always to the latest binaries regardless from which branch we run this.
I don't know how this could break running tests let's say from beta. I just wanted to pointed out. Maybe we should add a comment.

It seems it changed few times this year.
https://hg.mozilla.org/mozilla-central/log/e5effeb8e57c/testing/config/tooltool-manifests/linux32/releng.manifest
Attachment #8665977 - Flags: feedback?(armenzg) → feedback+
Comment on attachment 8665977 [details] [diff] [review]
crash symbols v1

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

::: testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ +45,4 @@
>          'help': 'absolute path to directory containing breakpad '
>                  'symbols, or the url of a zip file containing symbols.',
>      }],
> +] + copy.deepcopy(testing_config_options)

The test package url we will need soon when we have our tests located in the tree. So we would need nearly all of them, yes.

@@ +192,5 @@
> +        manifest_path = None
> +
> +        if self.config.get('download_minidump_stackwalk'):
> +            tooltool_manifest = self.query_minidump_tooltool_manifest()
> +            url_base = 'https://hg.mozilla.org/mozilla-central/raw-file/default/testing/'

Lets ask Ted what he thinks about it.
Flags: needinfo?(ted)
So I filed bug 1209011 to map all of our branches 1:1 with the Firefox/Gecko branches. With that information we could make the manifest path flexible to download a branch specific version of the minidump tool if really necessary. But I would still prefer Ted's feedback here.
Blocks: 1209209
Comment on attachment 8665977 [details] [diff] [review]
crash symbols v1

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

We agreed on the general implementation and simply waiting for a feedback from Ted. If we don't get it in time I would suggest that we land the current code and come up with a follow-up bug which implements branch based minidump stackwalk downloads.
Attachment #8665977 - Flags: review?(armenzg)
Comment on attachment 8665977 [details] [diff] [review]
crash symbols v1

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

I was going to ask for a try run for the testbase change but I see it will be deal with in bug 1208431.
Once that bug is fixed rebase over that and land.
Attachment #8665977 - Flags: review?(armenzg) → review+
Adding leave-open so we can still get Ted's feedback and maybe do a follow-up push.
Keywords: leave-open
Sorry for the delay. That seems fine. We used to have the binaries in the build/tools repo, and we used the same version everywhere. Obviously it's nicer when everything uses binaries from the same branch they're running on, so that changes ride the trains appropriately, but I don't think this will hurt anything.
Flags: needinfo?(ted)
Thanks Ted. I think we will leave that for now as it is then. When something should be broken we can still get this fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: