Bug 210975 - update-test-expectations-from-bugzilla tool not working with new EWS
Summary: update-test-expectations-from-bugzilla tool not working with new EWS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-24 09:52 PDT by Carlos Alberto Lopez Perez
Modified: 2020-05-20 07:09 PDT (History)
9 users (show)

See Also:


Attachments
Patch (31.60 KB, patch)
2020-05-11 16:25 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (31.64 KB, patch)
2020-05-15 13:49 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (31.67 KB, patch)
2020-05-18 07:43 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (33.41 KB, patch)
2020-05-18 20:02 PDT, Carlos Alberto Lopez Perez
jbedard: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2020-04-24 09:52:40 PDT
Since the move to the move to the new EWS the tool update-test-expectations-from-bugzilla doesn't work anymore.
This is caused because the news EWS doesn't upload anymore the layout test results as attachments to bugzilla.
Comment 1 Carlos Alberto Lopez Perez 2020-05-11 16:25:23 PDT
Created attachment 399067 [details]
Patch
Comment 2 youenn fablet 2020-05-15 08:32:51 PDT
Comment on attachment 399067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399067&action=review

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:91
> +            attachment = [attachment for attachment in bug_info.attachments(include_obsolete=False) if attachment.is_patch()]

s/attachment/attachments/

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:120
> +    def _get_layout_tests_run(self, bot_name, ews_build_url):

I am wondering whether other scripts might want to get the layout test results.
If so, maybe this should be added to some existing bot specific class?
Comment 3 Carlos Alberto Lopez Perez 2020-05-15 13:35:23 PDT
(In reply to youenn fablet from comment #2)
> Comment on attachment 399067 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399067&action=review
> 
> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:91
> > +            attachment = [attachment for attachment in bug_info.attachments(include_obsolete=False) if attachment.is_patch()]
> 
> s/attachment/attachments/
> 

Ok

> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:120
> > +    def _get_layout_tests_run(self, bot_name, ews_build_url):
> 
> I am wondering whether other scripts might want to get the layout test
> results.
> If so, maybe this should be added to some existing bot specific class?

I'm currently not aware of any other script wanting to get this results.
Do you know about anyone?

I have plans to keep improving this script and end re-using it in the bot that I proposed a while ago to help to automate updating WPT imports <https://lists.webkit.org/pipermail/webkit-dev/2020-March/031121.html>

For example, I would like that it can automatically update the TestExpectations files for tests failing with ImageOnlyFailure (or even with Crash).

But, it's something I plan to do iteratively. For now this patch only tries to make this script work back with the new EWS setup.
Comment 4 Carlos Alberto Lopez Perez 2020-05-15 13:49:03 PDT
Created attachment 399508 [details]
Patch
Comment 5 youenn fablet 2020-05-18 00:34:28 PDT
> I'm currently not aware of any other script wanting to get this results.
> Do you know about anyone?

I am not aware of this.
Jonathan or Aakash might have some insights there.

> I have plans to keep improving this script and end re-using it in the bot
> that I proposed a while ago to help to automate updating WPT imports
> <https://lists.webkit.org/pipermail/webkit-dev/2020-March/031121.html>
> 
> For example, I would like that it can automatically update the
> TestExpectations files for tests failing with ImageOnlyFailure (or even with
> Crash).
> 
> But, it's something I plan to do iteratively. For now this patch only tries
> to make this script work back with the new EWS setup.

Sounds good.
I'll review it tomorrow if nobody reviews it by then.
Comment 6 Jonathan Bedard 2020-05-18 07:11:55 PDT
Comment on attachment 399508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399508&action=review

Aakash definitely needs to lock at this before it lands, he is the EWS expert.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:43
> +import requests

Lets use the auto-installed version of requests

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:82
> +    def __init__(self, host, bugzilla_id, is_bug_id=True, bot_filter_name=None, attachment_fetcher=bugzilla.Bugzilla(), unzip=None, url_fetcher=requests):

Is there ever a world where we use a different url fetcher? Seems unlikely, requests is the blessed fetcher in Python 3

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:121
> +        if not re.match("http.*webkit.org/#/builders/[0-9]+/builds/[0-9]+$", ews_build_url):

Seems like this would match build.webkit.org too, at least once we move to a newer buildbot. Is that desirable?
Comment 7 Carlos Alberto Lopez Perez 2020-05-18 07:33:19 PDT
(In reply to Jonathan Bedard from comment #6)
> Comment on attachment 399508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399508&action=review
> 
> Aakash definitely needs to lock at this before it lands, he is the EWS
> expert.
> 
> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:43
> > +import requests
> 
> Lets use the auto-installed version of requests
> 

ok

> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:82
> > +    def __init__(self, host, bugzilla_id, is_bug_id=True, bot_filter_name=None, attachment_fetcher=bugzilla.Bugzilla(), unzip=None, url_fetcher=requests):
> 
> Is there ever a world where we use a different url fetcher? Seems unlikely,
> requests is the blessed fetcher in Python 3
> 

In the unit tests, I mock requests with a custom class to allow testing with a set of custom hand-made http requests-and-replies.

> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:121
> > +        if not re.match("http.*webkit.org/#/builders/[0-9]+/builds/[0-9]+$", ews_build_url):
> 
> Seems like this would match build.webkit.org too, at least once we move to a
> newer buildbot. Is that desirable?

My first idea was not being too strict with that sanity check. Right now the EWS server its at ews-build.webkit.org, but I don't know if it will be ever moved to $anything.webkit.org So this only tries to validate that the domain is webkit.org and that the URI path its in the format expected
Comment 8 Carlos Alberto Lopez Perez 2020-05-18 07:43:06 PDT
Created attachment 399645 [details]
Patch
Comment 9 Carlos Alberto Lopez Perez 2020-05-18 07:46:51 PDT
Updated patch to use auto-istalled requests module.

Note: the webkitpy failure on the EWS its unrelated to this.
Comment 10 Jonathan Bedard 2020-05-18 09:12:21 PDT
(In reply to Carlos Alberto Lopez Perez from comment #7)
> (In reply to Jonathan Bedard from comment #6)
> > Comment on attachment 399508 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=399508&action=review
> > 
> > Aakash definitely needs to lock at this before it lands, he is the EWS
> > expert.
> > 
> > > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:43
> > > +import requests
> > 
> > Lets use the auto-installed version of requests
> > 
> 
> ok
> 
> > > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:82
> > > +    def __init__(self, host, bugzilla_id, is_bug_id=True, bot_filter_name=None, attachment_fetcher=bugzilla.Bugzilla(), unzip=None, url_fetcher=requests):
> > 
> > Is there ever a world where we use a different url fetcher? Seems unlikely,
> > requests is the blessed fetcher in Python 3
> > 
> 
> In the unit tests, I mock requests with a custom class to allow testing with
> a set of custom hand-made http requests-and-replies.
> 

I won't block landing on this, but my preference is to use the mock module so that we keep production code clean of testing idioms.

webkitpy/results/upload_unittest.py has an example of this for requests.port, it looks something like this:

class MockResponse(object):
    def __init__(self, status_code=200, text=''):
        self.status_code = status_code
         self.text = text

with mock.patch('requests.post', new=lambda url, headers={}, data={}: self.MockResponse(status_code=200, text='data)):
    # Testing code
Comment 11 Carlos Alberto Lopez Perez 2020-05-18 20:01:22 PDT
(In reply to Jonathan Bedard from comment #10)
> > > Is there ever a world where we use a different url fetcher? Seems unlikely,
> > > requests is the blessed fetcher in Python 3
> > > 
> > 
> > In the unit tests, I mock requests with a custom class to allow testing with
> > a set of custom hand-made http requests-and-replies.
> > 
> 
> I won't block landing on this, but my preference is to use the mock module
> so that we keep production code clean of testing idioms.
> 

Ok. I will change this to use the mock module.
Comment 12 Carlos Alberto Lopez Perez 2020-05-18 20:02:18 PDT
Created attachment 399701 [details]
Patch
Comment 13 Aakash Jain 2020-05-19 10:48:40 PDT
Comment on attachment 399701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399701&action=review

I had a quick look at the high-level logic, looks good to me.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:122
> +        ews_buildbot_server, ews_buildbot_path = ews_build_url.split('#')

That's clever, getting the url from api data and modifying it appropriately. Also, if required, you can change the API output as well (at https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-app/ews/views/status.py#L41)
Comment 14 Carlos Alberto Lopez Perez 2020-05-19 13:43:26 PDT
(In reply to Aakash Jain from comment #13)
> Comment on attachment 399701 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399701&action=review
> 
> I had a quick look at the high-level logic, looks good to me.
> 
> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:122
> > +        ews_buildbot_server, ews_buildbot_path = ews_build_url.split('#')
> 
> That's clever, getting the url from api data and modifying it appropriately.

Thanks!

> Also, if required, you can change the API output as well (at
> https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-app/
> ews/views/status.py#L41)

I see, good to know that.
The current schema for the API its ok.
Comment 15 EWS 2020-05-20 06:29:13 PDT
Committed r261914: <https://trac.webkit.org/changeset/261914>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399701 [details].
Comment 16 Radar WebKit Bug Importer 2020-05-20 06:30:17 PDT
<rdar://problem/63444566>