Bug 221981 – [LayoutTests] Convert http/tests/misc convert PHP to Python

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

[Chris Gambrell]

Replacing PHP with equivalent Python CGI scripts

[Radar WebKit Bug Importer]

<rdar://problem/74399686>

[Chris Gambrell]

Created attachment 420693 [details]
Patch

[Chris Gambrell]

Created attachment 420714 [details]
Patch

[Chris Gambrell]

Created attachment 420724 [details]
Patch

[Chris Gambrell]

Created attachment 420728 [details]
Patch

[Chris Gambrell]

Created attachment 420750 [details]
Patch

[Chris Gambrell]

Created attachment 420792 [details]
Patch

[Jonathan Bedard]

Comment on attachment 420792 [details]
Patch

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

> LayoutTests/http/tests/misc/resources/charset-sniffer-end-sniffing.py:15
> +for i in range(0, 10000):

The usual idiom when you have a variable that you need to complete a statement but that you are not actually going to use is to use '_'

> LayoutTests/http/tests/misc/resources/check-test-file.py:28
> +        'PASS: File upload was successful\n'

Is there any "error" equivalent in Python? This code isn't exactly the same, although it's the same in the successful case, so that's probably sufficient.

> LayoutTests/http/tests/misc/resources/check-unnamed-file-included-in-formdata.py:23
> +if data != None:

Style is to use "data is not None" in this case.

> LayoutTests/http/tests/misc/resources/redirect-to-http-url.py:1
> +#!/usr/bin/env python3

Is there a php file being removed here?

> LayoutTests/http/tests/misc/resources/referrer-main-resource.py:6
> +referer = os.environ.get('HTTP_REFERER', '')

Don't need the default empty string (since you aren't printing it) and you probably don't need the variable either and could just put the os.environ.get in the if statement

> LayoutTests/http/tests/misc/resources/referrer-result.py:14
> +# formatting string

Remove this comment. I suspect the issue is about the brackets in the string, and can be mitigated with double brackets, but it's not a huge issue if these stay as "prints"

> LayoutTests/http/tests/misc/resources/stylesheet-bad-mime-type.py:18
> +    sys.stdout.write('\r\n')

I think we want the 406 status code here

[Chris Gambrell]

Created attachment 420871 [details]
Patch

[Chris Gambrell]

Created attachment 420886 [details]
Patch

[Chris Gambrell]

Created attachment 420917 [details]
Patch

[Chris Gambrell]

Created attachment 421001 [details]
Patch

[Chris Gambrell]

Created attachment 421010 [details]
Patch

[Chris Gambrell]

Created attachment 421018 [details]
Patch

[Chris Gambrell]

Created attachment 421019 [details]
Patch

[Chris Gambrell]

Created attachment 421021 [details]
Patch

[Chris Gambrell]

Created attachment 421048 [details]
Patch

[Chris Gambrell]

(In reply to Jonathan Bedard from comment #8)
> Comment on attachment 420792 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420792&action=review
> 
> > LayoutTests/http/tests/misc/resources/charset-sniffer-end-sniffing.py:15
> > +for i in range(0, 10000):
> 
> The usual idiom when you have a variable that you need to complete a
> statement but that you are not actually going to use is to use '_'
> 
> > LayoutTests/http/tests/misc/resources/check-test-file.py:28
> > +        'PASS: File upload was successful\n'
> 
> Is there any "error" equivalent in Python? This code isn't exactly the same,
> although it's the same in the successful case, so that's probably sufficient.
> 
> > LayoutTests/http/tests/misc/resources/check-unnamed-file-included-in-formdata.py:23
> > +if data != None:
> 
> Style is to use "data is not None" in this case.
> 
> > LayoutTests/http/tests/misc/resources/redirect-to-http-url.py:1
> > +#!/usr/bin/env python3
> 
> Is there a php file being removed here?
> 
> > LayoutTests/http/tests/misc/resources/referrer-main-resource.py:6
> > +referer = os.environ.get('HTTP_REFERER', '')
> 
> Don't need the default empty string (since you aren't printing it) and you
> probably don't need the variable either and could just put the
> os.environ.get in the if statement
> 
> > LayoutTests/http/tests/misc/resources/referrer-result.py:14
> > +# formatting string
> 
> Remove this comment. I suspect the issue is about the brackets in the
> string, and can be mitigated with double brackets, but it's not a huge issue
> if these stay as "prints"
> 
> > LayoutTests/http/tests/misc/resources/stylesheet-bad-mime-type.py:18
> > +    sys.stdout.write('\r\n')
> 
> I think we want the 406 status code here

Fixed in comment 17

[Chris Gambrell]

Created attachment 421054 [details]
Patch

[Chris Gambrell]

Created attachment 421061 [details]
Patch

[Chris Gambrell]

Created attachment 421189 [details]
Patch

[Chris Gambrell]

Created attachment 421193 [details]
Patch

[Chris Gambrell]

Created attachment 421204 [details]
Patch

[Chris Gambrell]

Created attachment 421208 [details]
Patch

[Chris Gambrell]

Created attachment 421213 [details]
Patch

[Chris Gambrell]

Created attachment 421237 [details]
Patch

[Chris Gambrell]

Created attachment 421282 [details]
Patch

[Chris Gambrell]

Created attachment 421309 [details]
Patch

[Chris Gambrell]

Created attachment 421351 [details]
Patch

[Chris Gambrell]

(In reply to Chris Gambrell from comment #29)
> Created attachment 421351 [details]
> Patch

There are still 3 PHP files remaining. Talked to Jonathan and it is beneficial to proceed with committing the changes that are complete and move on to more directories to keep progressing through the files we need to convert.

This directory will be revisited later. After cq+ the bug should remain open and not resolved.

LayoutTests/http/tests/misc/resources/redirect-to-http-url.php
LayoutTests/http/tests/misc/resources/prefetch-purpose.php
LayoutTests/http/tests/misc/resources/check-query-param.php

[Jonathan Bedard]

Comment on attachment 421351 [details]
Patch

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

> LayoutTests/http/tests/loading/resources/slowimage.py:1
> +#!/usr/bin/env python3

I don't see the file that this is replacing, and it looks like a duplicate?

> LayoutTests/http/tests/misc/refresh-headers.py:11
> +if cache_control.lower() == 'no-cache' or pragma.lower == 'no-cache':

Should be pragma.lower()

> LayoutTests/http/tests/misc/resources/char-encoding-in-hidden-charset-field.py:7
> +data = ''.join(sys.stdin.readlines())

It seems wrong to read data if our request method is not a POST request

> LayoutTests/http/tests/misc/resources/check-query-param.py:1
> +#!/usr/bin/env python3

I don't see the PHP file we are replacing removed.

> LayoutTests/http/tests/misc/resources/dns-prefetch-control.py:10
> +sys.stdout.write('Content-Type: text/html\r\n\r\n')

X-DNS-Prefetch-Control is part of the header. You are ending the header here.

> LayoutTests/http/tests/misc/resources/dns-prefetch-control.py:17
> +    sys.stdout.write('X-DNS-Prefetch-Control: foo\r\n\r\n')

Maybe just:
if value in ['on', 'off', 'foo']:
    sys.stdout.write('X-DNS-Prefetch-Control: off\r\n'.format(value))

> LayoutTests/http/tests/misc/resources/image-checks-for-accept.py:6
> +allimages = 'image/*'

This doesn't really need be a variable.

> LayoutTests/http/tests/misc/resources/webtiming-cross-origin-and-back-redirect2.php:-2
> -  // 127.0.0.1 is where the test originally started. We redirected to "localhost" before this.

I would keep this comment in the Python file

> LayoutTests/http/tests/misc/resources/protected/protected-image.py:15
> +if !username:

Use `not username`

[Chris Gambrell]

Created attachment 421859 [details]
Patch

[Chris Gambrell]

Created attachment 421861 [details]
Patch

[Chris Gambrell]

Created attachment 421892 [details]
Patch

[Chris Gambrell]

Created attachment 421937 [details]
Patch

[EWS]

Committed r273819: <https://commits.webkit.org/r273819>

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

[Chris Gambrell]

Reopening for second pass

[Chris Gambrell]

Created attachment 424985 [details]
Patch

[Chris Gambrell]

Created attachment 424986 [details]
Patch

[Chris Gambrell]

Created attachment 424987 [details]
Patch

[Chris Gambrell]

Comment on attachment 424987 [details]
Patch

apply-patch is having issues processing http/tests/misc/will-send-request-returns-null-on-redirect.html's resources being converted from .php to .py. No other tests use http/tests/misc/resources/redirect-to-http-url.php so I have included the .py version without removing the .php file so that EWS can chew on the other converted files. 

The test file that references http/tests/misc/resources/redirect-to-http-url.py (the file that is having trouble going through apply-patch) passes locally on wk1 and wk2. 

platform/wk2/http/tests/misc/will-send-request-returns-null-on-redirect-expected.txt will need to be updated along with http/tests/misc/will-send-request-returns-null-on-redirect.html and http/tests/misc/will-send-request-returns-null-on-redirect-expected.txt. 

Once r+, I will remove the .php version and update the files in question to reflect the .py version instead of the .php and will land it manually to bypass the apply-patch issues

[Jonathan Bedard]

Comment on attachment 424987 [details]
Patch

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

> LayoutTests/http/tests/misc/resources/check-query-param.py:21
> +if 'HTTP_COOKIE' in os.environ:

Might consider using get_cookies and request.update()

> LayoutTests/http/tests/misc/resources/redirect-to-http-url.py:1
> +#!/usr/bin/env python3

Where is the php file we're replacing? Is it still used elsewhere?

[Chris Gambrell]

Created attachment 425034 [details]
Patch

[Chris Gambrell]

(In reply to Jonathan Bedard from comment #42)
> Comment on attachment 424987 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424987&action=review
> 
> > LayoutTests/http/tests/misc/resources/check-query-param.py:21
> > +if 'HTTP_COOKIE' in os.environ:
> 
> Might consider using get_cookies and request.update()
> 
> > LayoutTests/http/tests/misc/resources/redirect-to-http-url.py:1
> > +#!/usr/bin/env python3
> 
> Where is the php file we're replacing? Is it still used elsewhere?

See comment 41

[Jonathan Bedard]

Comment on attachment 425034 [details]
Patch

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

> LayoutTests/http/tests/misc/resources/check-query-param.py:29
> +    request.update({ key: cookies[key] })

Don't need a loop here, request.update(get_cookies()) should work

[Chris Gambrell]

Created attachment 425036 [details]
Patch

[Chris Gambrell]

Committed r275561 (236216@main): <https://commits.webkit.org/236216@main>