Bug 210975 – update-test-expectations-from-bugzilla tool not working with new EWS

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

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.

[Carlos Alberto Lopez Perez]

Created attachment 399067 [details]
Patch

[youenn fablet]

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?

[Carlos Alberto Lopez Perez]

(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.

[Carlos Alberto Lopez Perez]

Created attachment 399508 [details]
Patch

[youenn fablet]

> 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.

[Jonathan Bedard]

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?

[Carlos Alberto Lopez Perez]

(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

[Carlos Alberto Lopez Perez]

Created attachment 399645 [details]
Patch

[Carlos Alberto Lopez Perez]

Updated patch to use auto-istalled requests module.

Note: the webkitpy failure on the EWS its unrelated to this.

[Jonathan Bedard]

(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

[Carlos Alberto Lopez Perez]

(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.

[Carlos Alberto Lopez Perez]

Created attachment 399701 [details]
Patch

[Aakash Jain]

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)

[Carlos Alberto Lopez Perez]

(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.

[EWS]

Committed r261914: <https://trac.webkit.org/changeset/261914>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399701 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63444566>