Bug 222198 – [LayoutTests] Convert http/tests/loading 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/74536576>

[Chris Gambrell]

Created attachment 421349 [details]
Patch

[Chris Gambrell]

Created attachment 421355 [details]
Patch

[Chris Gambrell]

Created attachment 421411 [details]
Patch

[Chris Gambrell]

Created attachment 421419 [details]
Patch

[Jonathan Bedard]

Comment on attachment 421419 [details]
Patch

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

> LayoutTests/http/tests/loading/authentication-after-redirect-stores-wrong-credentials/resources/wrong-credential-1-redirect-to-auth.php:-3
> -// That is important, and the next page has to be loaded using 127.0.0.1.

I think you should retain this comment in the python code, even though it isn't in the request

> LayoutTests/http/tests/loading/resourceLoadStatistics/resources/set-cookie.php:-9
> -</script>

This file is being removed, but not replaced. Is it not used anywhere? If not, it's fine to remove it (it's a resource, so it is not itself a test)

> LayoutTests/http/tests/loading/resources/imported-stylesheet-varying-according-domain.css.py:10
> +color = ''

You can do this in a one-liner:

color = 'green' if host == domain else 'red'

> LayoutTests/http/tests/loading/resources/imported-stylesheet-varying-according-domain.css.py:19
> +    'body {{ background-color: {};}}'.format(color)

You may actually consider putting the one-liner as the format argument

> LayoutTests/http/tests/loading/resources/post-in-iframe-with-back-navigation-page-1.py:6
> +today = time.time()

Nit: I'd call this variable "now" since it isn't just the day

> LayoutTests/http/tests/loading/resources/post-in-iframe-with-back-navigation-page-2.py:6
> +today = time.time()

Nit: I'd call this variable "now" since it isn't just the day

> LayoutTests/http/tests/loading/resources/post-in-iframe-with-back-navigation-page-3.py:6
> +today = time.time()

Nit: I'd call this variable "now" since it isn't just the day

[Chris Gambrell]

Created attachment 421835 [details]
Patch

[Chris Gambrell]

(In reply to Jonathan Bedard from comment #6)
> Comment on attachment 421419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421419&action=review
> 
> > LayoutTests/http/tests/loading/authentication-after-redirect-stores-wrong-credentials/resources/wrong-credential-1-redirect-to-auth.php:-3
> > -// That is important, and the next page has to be loaded using 127.0.0.1.
> 
> I think you should retain this comment in the python code, even though it
> isn't in the request
> 
> > LayoutTests/http/tests/loading/resourceLoadStatistics/resources/set-cookie.php:-9
> > -</script>
> 
> This file is being removed, but not replaced. Is it not used anywhere? If
> not, it's fine to remove it (it's a resource, so it is not itself a test)
> 
> > LayoutTests/http/tests/loading/resources/imported-stylesheet-varying-according-domain.css.py:10
> > +color = ''
> 
> You can do this in a one-liner:
> 
> color = 'green' if host == domain else 'red'
> 
> > LayoutTests/http/tests/loading/resources/imported-stylesheet-varying-according-domain.css.py:19
> > +    'body {{ background-color: {};}}'.format(color)
> 
> You may actually consider putting the one-liner as the format argument
> 
> > LayoutTests/http/tests/loading/resources/post-in-iframe-with-back-navigation-page-1.py:6
> > +today = time.time()
> 
> Nit: I'd call this variable "now" since it isn't just the day
> 
> > LayoutTests/http/tests/loading/resources/post-in-iframe-with-back-navigation-page-2.py:6
> > +today = time.time()
> 
> Nit: I'd call this variable "now" since it isn't just the day
> 
> > LayoutTests/http/tests/loading/resources/post-in-iframe-with-back-navigation-page-3.py:6
> > +today = time.time()
> 
> Nit: I'd call this variable "now" since it isn't just the day

Fixed in comment 7

[Chris Gambrell]

Created attachment 421842 [details]
Patch

[EWS]

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

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

[Chris Gambrell]

Reopening for the second pass

[Chris Gambrell]

Created attachment 424865 [details]
Patch

[Chris Gambrell]

Created attachment 424974 [details]
Patch

[Jonathan Bedard]

Comment on attachment 424974 [details]
Patch

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

> LayoutTests/http/tests/loading/resources/redirect-methods-result.py:24
> +    header_cookies = os.environ['HTTP_COOKIE']

Any reason not to use get_cookies()?

[Chris Gambrell]

Created attachment 425038 [details]
Patch

[EWS]

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

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