Summary: | update-test-expectations-from-bugzilla tool not working with new EWS | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||||||||
Component: | Tools / Tests | Assignee: | Carlos Alberto Lopez Perez <clopez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aakash_jain, ap, dewei_zhu, ews-watchlist, glenn, jbedard, rniwa, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Carlos Alberto Lopez Perez
2020-04-24 09:52:40 PDT
Created attachment 399067 [details]
Patch
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? (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. Created attachment 399508 [details]
Patch
> 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 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? (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 Created attachment 399645 [details]
Patch
Updated patch to use auto-istalled requests module. Note: the webkitpy failure on the EWS its unrelated to this. (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 (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. Created attachment 399701 [details]
Patch
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) (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. Committed r261914: <https://trac.webkit.org/changeset/261914> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399701 [details]. |