Bug 222935

Summary: [LayoutTests] Convert http/tests/cache 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
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch jbedard: review+

Description Chris Gambrell 2021-03-08 12:53:25 PST
Replacing PHP with equivalent Python CGI scripts
Comment 1 Radar WebKit Bug Importer 2021-03-08 12:54:37 PST
<rdar://problem/75183314>
Comment 2 Chris Gambrell 2021-03-10 19:48:42 PST
Created attachment 422893 [details]
Patch
Comment 3 Chris Gambrell 2021-03-10 19:58:26 PST
Created attachment 422895 [details]
Patch
Comment 4 Chris Gambrell 2021-03-11 08:23:58 PST
Created attachment 422933 [details]
Patch
Comment 5 Chris Gambrell 2021-03-11 15:08:15 PST
Created attachment 422977 [details]
Patch
Comment 6 Chris Gambrell 2021-03-11 15:21:03 PST
Created attachment 422981 [details]
Patch
Comment 7 Chris Gambrell 2021-03-12 09:13:58 PST
Created attachment 423056 [details]
Patch
Comment 8 Chris Gambrell 2021-03-15 11:28:01 PDT
Created attachment 423204 [details]
Patch
Comment 9 Jonathan Bedard 2021-03-15 15:13:58 PDT
Comment on attachment 423204 [details]
Patch

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

Biggest note is that I don't think our Windows bots have f strings available, although we can let EWS check that assumption

> LayoutTests/http/tests/cache/post-redirect-get.php:-4
> -// This test loads an uncacheable main resource and a cacheable image subresource.

We should keep this comment around

> LayoutTests/http/tests/cache/post-redirect-get.py:12
> +sys.path.insert(0, http_root)

No reason to do this if we aren't using any shared code

> LayoutTests/http/tests/cache/post-redirect-get.py:27
> +    f'Expires: {expires}\r\n'

f strings are introduced in Python 3.6....I think our Windows bots are using 3.5

> LayoutTests/http/tests/cache/post-redirect-get.py:40
> +    '<html>\r\n'

\r\n isn't going to be harmful here, but is there a reason we're doing \r\n instead of just \n?

> LayoutTests/http/tests/cache/post-with-cached-subresources.py:18
> +sys.path.insert(0, http_root)

We don't need this since we aren't importing any shared code

> LayoutTests/http/tests/cache/reload-main-resource.py:9
> +sys.path.insert(0, http_root)

We don't need this since we aren't importing any shared code

> LayoutTests/http/tests/cache/disk-cache/resources/redirect-chain.py:24
> +    f'Location: redirect-chain.py?length={length}&uniqueId={unique_id}&random={randint(0, sys.maxsize)}\r\n\r\n'

I think we need to avoid f strings because of Windows for the moment

> LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/validation-request-frame.py:12
> +    f'Set-Cookie: {cookie}\r\n\r\n'

I think we should avoid f string for the sake of our Windows bots

> LayoutTests/http/tests/cache/resources/cacheable-iframe.py:10
> +    f'Cache-Control: public, max-age={max_age}\r\n'

Ditto on the f strings

> LayoutTests/http/tests/cache/resources/load-and-check-referer.py:7
> +def contentType(path):

Existing function is fine as is, but might consider doing something like this:
{
    'html': 'text/html',
    'manifest': 'text/cache-manifest',
    ...
}.get(path.split('.')[-1], 'text/plain')

> LayoutTests/http/tests/cache/resources/partitioned-cache-echo-state.py:9
> +sys.path.insert(0, http_root)

We aren't using any shared code, so we don't need to edit the Python path

> LayoutTests/http/tests/cache/resources/post-image-to-verify.py:1
> +#!/usr/bin/env python3

Where is the file we're replacing?

> LayoutTests/http/tests/cache/resources/reload-main-resource-iframe.py:10
> +sys.path.insert(0, http_root)

We aren't using any shared code, so we don't need to edit the Python path

> LayoutTests/http/tests/cache/resources/stylesheet304.php:-21
> -?>

Doesn't look live we've replaced this file. Is it no longer in use?

> LayoutTests/http/tests/resources/touch-temp-file.py:1
> +#!/usr/bin/env python3

I don't see the file this is replacing.

> LayoutTests/http/tests/resources/touch-temp-file.py:10
> +sys.path.insert(0, http_root)

We aren't using any shared code, so we don't need to edit the Python path

> LayoutTests/http/tests/resources/write-temp-file.py:1
> +#!/usr/bin/env python3

I don't see the file we're replacing.

> LayoutTests/http/tests/resources/write-temp-file.py:10
> +sys.path.insert(0, http_root)

We aren't using any shared code, so we don't need to edit the Python path
Comment 10 Chris Gambrell 2021-03-16 11:44:58 PDT
Created attachment 423370 [details]
Patch
Comment 11 Chris Gambrell 2021-03-16 11:51:01 PDT
Comment on attachment 423370 [details]
Patch

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

> LayoutTests/http/tests/cache/post-redirect-get.py:46
> +    '<html>\n'

Fixed \r\n to \n

> LayoutTests/http/tests/cache/resources/load-and-check-referer.py:7
> +def contentType(path):

Converted func to your dictionary approach. Got errors when running test including images. Will look closer into changing to a dictionary approach the second pass with this directory with the more challenging conversions.

> LayoutTests/http/tests/cache/resources/post-image-to-verify.py:1
> +#!/usr/bin/env python3

Removing PHP equivalent.

> LayoutTests/http/tests/cache/resources/stylesheet304.php:-1
> -<?php

No need to replace. No longer in use

> LayoutTests/http/tests/resources/touch-temp-file.py:1
> +#!/usr/bin/env python3

Still used by multiple .php files. This directory/tests are reference throughout http/tests so this will be the last directory for full conversion.

> LayoutTests/http/tests/resources/write-temp-file.py:1
> +#!/usr/bin/env python3

Still used by multiple .php files. This directory/tests are reference throughout http/tests so this will be the last directory for full conversion.
Comment 12 Chris Gambrell 2021-03-16 11:52:24 PDT
(In reply to Chris Gambrell from comment #11)
> Comment on attachment 423370 [details]
> Patch
> 

Waiting to convert the f strings back to regular strings with the .format modifier since previous patch passed EWS. If the conversion isn't needed, will leave it with f strings. If conversion is needed, I will push another patch.
Comment 13 Jonathan Bedard 2021-03-16 15:18:09 PDT
(In reply to Chris Gambrell from comment #12)
> (In reply to Chris Gambrell from comment #11)
> > Comment on attachment 423370 [details]
> > Patch
> > 
> 
> Waiting to convert the f strings back to regular strings with the .format
> modifier since previous patch passed EWS. If the conversion isn't needed,
> will leave it with f strings. If conversion is needed, I will push another
> patch.

Looks like I was wrong about the version of Python our Windows bots are running. The are running 3.6, we can use f-strings.
Comment 14 EWS 2021-03-17 11:56:09 PDT
Committed r274574: <https://commits.webkit.org/r274574>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423370 [details].
Comment 15 Chris Gambrell 2021-03-24 08:46:06 PDT
Reopening to complete conversion of directory
Comment 16 Chris Gambrell 2021-03-24 08:56:55 PDT
Created attachment 424137 [details]
Patch
Comment 17 Chris Gambrell 2021-03-24 09:11:09 PDT
Created attachment 424139 [details]
Patch
Comment 18 Chris Gambrell 2021-03-24 10:14:43 PDT
Created attachment 424151 [details]
Patch
Comment 19 Jonathan Bedard 2021-03-31 13:25:47 PDT
Comment on attachment 424151 [details]
Patch

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

> LayoutTests/http/tests/cache/resources/cache-control-redirect.py:10
> +code = query.get('code', [None])[-1]

Should replace None with 302 and drop the if statement on line 15

> LayoutTests/http/tests/cache/resources/x-frame-options.py:1
> +#!/usr/bin/env python3

I don't see the file we are replacing
Comment 20 Chris Gambrell 2021-03-31 18:34:25 PDT
Created attachment 424856 [details]
Patch
Comment 21 Chris Gambrell 2021-03-31 18:34:55 PDT
(In reply to Jonathan Bedard from comment #19)
> Comment on attachment 424151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424151&action=review
> 
> > LayoutTests/http/tests/cache/resources/cache-control-redirect.py:10
> > +code = query.get('code', [None])[-1]
> 
> Should replace None with 302 and drop the if statement on line 15
> 
> > LayoutTests/http/tests/cache/resources/x-frame-options.py:1
> > +#!/usr/bin/env python3
> 
> I don't see the file we are replacing

Fixed in Comment 20
Comment 22 Chris Gambrell 2021-04-01 14:33:27 PDT
Created attachment 424949 [details]
Patch
Comment 23 Chris Gambrell 2021-04-01 14:37:22 PDT
Comment on attachment 424949 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * http/tests/cache/resources/x-frame-options.py: Added.

PHP counterpart not removed because of an issue with it and apply-patch. When r+'d I will remove locally and land.
Comment 24 Jonathan Bedard 2021-04-01 15:39:08 PDT
Comment on attachment 424949 [details]
Patch

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

> LayoutTests/http/tests/cache/resources/x-frame-options.py:15
> +one_year = 12 * 31 * 24 * 60 * 60

Super weird way to represent a year, we usually use 365 instead of 12 * 31
Comment 25 Chris Gambrell 2021-04-01 18:07:37 PDT
Committed r275399 (236063@main): <https://commits.webkit.org/236063@main>