Bug 223891

Summary: [LayoutTests] Convert http/tests/cookies convert PHP to Python
Product: WebKit Reporter: Chris Gambrell <cgambrell>
Component: Tools / TestsAssignee: Chris Gambrell <cgambrell>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ews-watchlist, hi, jbedard, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=220749
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Gambrell 2021-03-29 11:56:38 PDT
Replacing PHP with equivalent Python CGI scripts
Comment 1 Radar WebKit Bug Importer 2021-03-29 11:57:40 PDT
<rdar://problem/75965634>
Comment 2 Chris Gambrell 2021-03-29 12:19:37 PDT
Created attachment 424563 [details]
Patch
Comment 3 Jonathan Bedard 2021-03-29 13:52:52 PDT
Comment on attachment 424563 [details]
Patch

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

> LayoutTests/http/tests/cookies/resources/cookie-utility.py:18
> +    expires= datetime.utcnow() - timedelta(seconds=86400)

Nit: Need space before =

> LayoutTests/http/tests/cookies/resources/cookie-utility.py:29
> +    sys.exit(0)

With the 'else' block, we don't need sys.exit(0)

> LayoutTests/http/tests/cookies/resources/cookie-utility.py:30
> +elif query_function == 'deleteCookies':

For readability, I would put empty lines before each elif.

> LayoutTests/http/tests/cookies/resources/set-cookie-and-serve.py:18
> +with open(os.path.join('/'.join(__file__.split('/')[0:-1]), destination), 'r') as file:

Instead of using ".split", you should use "os.path.dirname".

> LayoutTests/http/tests/cookies/resources/set-cookie-on-redirect.py:17
> +elif step == 2:

Nit: Put empty lines before the elif for readability

> LayoutTests/http/tests/cookies/resources/set-cookie-on-redirect.py:18
> +    expires= datetime.utcnow() + timedelta(seconds=86400)

Nit: Put space before =

> LayoutTests/http/tests/cookies/same-site/resources/fetch-after-top-level-cross-origin-redirect.py:7
> +cookies = {}

Not required, although I see us repeating this 7 line code a lot. Do you think if may be worth it to standardize this in LayoutTests/http/tests/resources/portabilityLayer.py?
Comment 4 Chris Gambrell 2021-03-30 12:11:34 PDT
Created attachment 424673 [details]
Patch
Comment 5 Chris Gambrell 2021-03-30 13:04:25 PDT
Created attachment 424679 [details]
Patch
Comment 6 Jonathan Bedard 2021-03-31 13:21:27 PDT
Comment on attachment 424679 [details]
Patch

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

> LayoutTests/http/tests/cookies/resources/cookie-utility.py:36
> +    expires= datetime.utcnow() + timedelta(seconds=86400)

Might need to search through the whole patch for cases where you don't have a space in front of the = in `expires=`
Comment 7 Chris Gambrell 2021-03-31 16:16:08 PDT
Committed r275315 (235996@main): <https://commits.webkit.org/235996@main>
Comment 8 Chris Gambrell 2021-03-31 16:16:40 PDT
(In reply to Jonathan Bedard from comment #6)
> Comment on attachment 424679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424679&action=review
> 
> > LayoutTests/http/tests/cookies/resources/cookie-utility.py:36
> > +    expires= datetime.utcnow() + timedelta(seconds=86400)
> 
> Might need to search through the whole patch for cases where you don't have
> a space in front of the = in `expires=`

Quite a few instances- fixed and committed
Comment 9 Chris Gambrell 2021-04-08 12:07:42 PDT
Reopening for the final PHP files that need to be converted
Comment 10 Chris Gambrell 2021-04-08 12:20:21 PDT
Created attachment 425529 [details]
Patch
Comment 11 Jonathan Bedard 2021-04-08 13:20:31 PDT
Comment on attachment 425529 [details]
Patch

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

> LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie.py:52
> +</html>'''.format(redirect_url(1)))

Style checker might complain, but we need to organize this string to retain the indent, this code is hard to read without indenting the function contents. The multi-line sys.stdout that you do for headers is probably the right technique.
Comment 12 Chris Gambrell 2021-04-08 15:56:11 PDT
Created attachment 425554 [details]
Patch
Comment 13 EWS 2021-04-09 08:02:32 PDT
Committed r275762 (236340@main): <https://commits.webkit.org/236340@main>

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