Bug 197519 – webkit-patch --no-review upload does not submit patch to New EWS

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

Created attachment 368790 [details]
Screenshot - Not submitted to New EWS

Ran `Tools/Scripts/webkit-patch upload -g HEAD --no-review 197480` today, here's the output, emphasis mine:

$ Tools/Scripts/webkit-patch upload -g HEAD --no-review 197480
Total errors found: 0 in 23 files
4 files to edit
Was that diff correct? [Y/n]: Y
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 [e-mail]...
Fetching: https://bugs.webkit.org/show_bug.cgi?id=197480&ctype=xml&excludefield=attachmentdata
Obsoleting 1 old patch on bug 197480
Obsoleting attachment: 368720
Fetching: https://bugs.webkit.org/show_bug.cgi?id=197480&ctype=xml&excludefield=attachmentdata
Adding patch "Patch" to https://bugs.webkit.org/show_bug.cgi?id=197480
Fetching: https://bugs.webkit.org/attachment.cgi?id=368787&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=197480&ctype=xml&excludefield=attachmentdata
***Submitting attachment 368787 [details] to EWS queues***

The patch was submitted to the old EWS, but was not submitted to the new EWS.

[Radar WebKit Bug Importer]

<rdar://problem/50424887>

[Aakash Jain]

Created attachment 368844 [details]
Patch

[Aakash Jain]

(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 [e-mail]...
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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jonathan Bedard]

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.

[Aakash Jain]

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.

[Aakash Jain]

Created attachment 368985 [details]
Patch

[Jonathan Bedard]

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

[Jonathan Bedard]

Looks good to me, unofficial r+.

[WebKit Commit Bot]

Comment on attachment 368985 [details]
Patch

Clearing flags on attachment: 368985

Committed r244923: <https://trac.webkit.org/changeset/244923>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.