Summary: | [LayoutTests] Convert http/tests/cache convert PHP to Python | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Gambrell <cgambrell> | ||||||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | 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
Chris Gambrell
2021-03-08 12:53:25 PST
Created attachment 422893 [details]
Patch
Created attachment 422895 [details]
Patch
Created attachment 422933 [details]
Patch
Created attachment 422977 [details]
Patch
Created attachment 422981 [details]
Patch
Created attachment 423056 [details]
Patch
Created attachment 423204 [details]
Patch
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 Created attachment 423370 [details]
Patch
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. (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. (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. Committed r274574: <https://commits.webkit.org/r274574> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423370 [details]. Reopening to complete conversion of directory Created attachment 424137 [details]
Patch
Created attachment 424139 [details]
Patch
Created attachment 424151 [details]
Patch
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 Created attachment 424856 [details]
Patch
(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 Created attachment 424949 [details]
Patch
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 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 Committed r275399 (236063@main): <https://commits.webkit.org/236063@main> |