| Summary: | webkit-patch --no-review upload does not submit patch to New EWS | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||
| Component: | Tools / Tests | Assignee: | Aakash Jain <aakash_jain> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, commit-queue, dewei_zhu, ews-watchlist, glenn, jbedard, lforschler, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Local Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Daniel Bates
2019-05-02 11:11:23 PDT
Created attachment 368844 [details]
Patch
(In reply to Aakash Jain from comment #2) > Created attachment 368844 [details] This patch was uploaded using the 'webkit-patch upload --no-review' command. It did submit the patch to both old and new EWS. Logs: [~/code/OpenSource/Tools]$webkit-patch upload --no-review 197519 Total errors found: 0 in 11 files Was that diff correct? [Y/n]: Reading Keychain for bugs.webkit.org account and password. Click "Allow" to continue... Could not find a keychain entry for bugs.webkit.org. Reading Keychain for bugs.webkit.org account and password. Click "Allow" to continue... Logging in as aakash_jain@apple.com... Fetching: https://bugs.webkit.org/show_bug.cgi?id=197519&ctype=xml&excludefield=attachmentdata Fetching: https://bugs.webkit.org/show_bug.cgi?id=197519&ctype=xml&excludefield=attachmentdata Adding patch "Patch" to https://bugs.webkit.org/show_bug.cgi?id=197519 Fetching: https://bugs.webkit.org/attachment.cgi?id=368844&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=197519&ctype=xml&excludefield=attachmentdata Submitting attachment 368844 [details] to EWS queues Submitting attachment 368844 [details] to EWS queues Comment on attachment 368844 [details] Patch Attachment 368844 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12070395 New failing tests: http/tests/css/filters-on-iframes.html Created attachment 368860 [details]
Archive of layout-test-results from ews212 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 368844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368844&action=review > Tools/ChangeLog:17 > + * Scripts/webkitpy/common/net/ewsserver_unittest.py: Added unit-test. Unit test for the EWSServer class or unit test for the MockEWSServer class? > Tools/Scripts/webkitpy/common/net/ewsserver.py:35 > + self.use_https = bool(use_https) Would it ever be valid to NOT use https here? > Tools/Scripts/webkitpy/common/net/ewsserver_mock.py:32 > + self.use_https = True Why do we need use_https? > Tools/Scripts/webkitpy/common/net/ewsserver_unittest.py:34 > + self.assertTrue(ews_server.use_https) This doesn't seem like a very useful test. I think we could actually try the submit_to_ews(...) function, I think that MockBrowser() provides enough to let that work. Comment on attachment 368844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368844&action=review >> Tools/ChangeLog:17 >> + * Scripts/webkitpy/common/net/ewsserver_unittest.py: Added unit-test. > > Unit test for the EWSServer class or unit test for the MockEWSServer class? For EWSServer class. >> Tools/Scripts/webkitpy/common/net/ewsserver.py:35 >> + self.use_https = bool(use_https) > > Would it ever be valid to NOT use https here? It might be valid if for some reason https is not configured properly on the server (e.g.: submitting the patches to a UAT instance of EWS). I can't think of another strong reason. I was trying to keep it consistent with similar statusserver code. use_https was added by Dan Bates in r232979. I believe he might have encountered a scenario in which there was a need to use http instead of https. >> Tools/Scripts/webkitpy/common/net/ewsserver_mock.py:32 >> + self.use_https = True > > Why do we need use_https? Removing in updated patch. >> Tools/Scripts/webkitpy/common/net/ewsserver_unittest.py:34 >> + self.assertTrue(ews_server.use_https) > > This doesn't seem like a very useful test. I think we could actually try the submit_to_ews(...) function, I think that MockBrowser() provides enough to let that work. Improved unit-test little bit, it now executes the submit_to_ews method. Note that upload_unittest.py also cover submit_to_ews to some extent. Created attachment 368985 [details]
Patch
(In reply to Aakash Jain from comment #7) ... > > > > Would it ever be valid to NOT use https here? > > It might be valid if for some reason https is not configured properly on the > server (e.g.: submitting the patches to a UAT instance of EWS). I can't > think of another strong reason. I was trying to keep it consistent with > similar statusserver code. use_https was added by Dan Bates in r232979. I > believe he might have encountered a scenario in which there was a need to > use http instead of https. I hadn't thought about UAT, that would be a good reason. Looks good to me, unofficial r+. Comment on attachment 368985 [details] Patch Clearing flags on attachment: 368985 Committed r244923: <https://trac.webkit.org/changeset/244923> All reviewed patches have been landed. Closing bug. |