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+

Chris Gambrell
Reported 2021-03-08 12:53:25 PST
Replacing PHP with equivalent Python CGI scripts
Attachments
Patch (102.52 KB, patch)
2021-03-10 19:48 PST, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (99.93 KB, patch)
2021-03-10 19:58 PST, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (96.48 KB, patch)
2021-03-11 08:23 PST, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (113.04 KB, patch)
2021-03-11 15:08 PST, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (109.92 KB, patch)
2021-03-11 15:21 PST, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (109.90 KB, patch)
2021-03-12 09:13 PST, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (109.99 KB, patch)
2021-03-15 11:28 PDT, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (111.08 KB, patch)
2021-03-16 11:44 PDT, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (10.40 KB, patch)
2021-03-24 08:56 PDT, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (10.40 KB, patch)
2021-03-24 09:11 PDT, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (9.33 KB, patch)
2021-03-24 10:14 PDT, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (10.30 KB, patch)
2021-03-31 18:34 PDT, Chris Gambrell 		no flags
DetailsFormatted DiffDiff
Patch (10.31 KB, patch)
2021-04-01 14:33 PDT, Chris Gambrell 		jbedard: review+
DetailsFormatted DiffDiff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-03-08 12:54:37 PST
<rdar://problem/75183314>
Chris Gambrell
Comment 2 2021-03-10 19:48:42 PST
Created attachment 422893 [details] Patch
Chris Gambrell
Comment 3 2021-03-10 19:58:26 PST
Created attachment 422895 [details] Patch
Chris Gambrell
Comment 4 2021-03-11 08:23:58 PST
Created attachment 422933 [details] Patch
Chris Gambrell
Comment 5 2021-03-11 15:08:15 PST
Created attachment 422977 [details] Patch
Chris Gambrell
Comment 6 2021-03-11 15:21:03 PST
Created attachment 422981 [details] Patch
Chris Gambrell
Comment 7 2021-03-12 09:13:58 PST
Created attachment 423056 [details] Patch
Chris Gambrell
Comment 8 2021-03-15 11:28:01 PDT
Created attachment 423204 [details] Patch
Jonathan Bedard
Comment 9 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
Chris Gambrell
Comment 10 2021-03-16 11:44:58 PDT
Created attachment 423370 [details] Patch
Chris Gambrell
Comment 11 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.
Chris Gambrell
Comment 12 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.
Jonathan Bedard
Comment 13 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.
EWS
Comment 14 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].
Chris Gambrell
Comment 15 2021-03-24 08:46:06 PDT
Reopening to complete conversion of directory
Chris Gambrell
Comment 16 2021-03-24 08:56:55 PDT
Created attachment 424137 [details] Patch
Chris Gambrell
Comment 17 2021-03-24 09:11:09 PDT
Created attachment 424139 [details] Patch
Chris Gambrell
Comment 18 2021-03-24 10:14:43 PDT
Created attachment 424151 [details] Patch
Jonathan Bedard
Comment 19 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
Chris Gambrell
Comment 20 2021-03-31 18:34:25 PDT
Created attachment 424856 [details] Patch
Chris Gambrell
Comment 21 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
Chris Gambrell
Comment 22 2021-04-01 14:33:27 PDT
Created attachment 424949 [details] Patch
Chris Gambrell
Comment 23 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.
Jonathan Bedard
Comment 24 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
Chris Gambrell
Comment 25 2021-04-01 18:07:37 PDT
Committed r275399 (236063@main): <https://commits.webkit.org/236063@main>
Note You need to log in before you can comment on or make changes to this bug.