Bug 220995 – [LayoutTests] Convert xmlhttprequest php to Python

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

[Jonathan Bedard]

Converting all php in xmlhttprequest to Python to gauge the difficulty of doing this across all layout tests.

[Radar WebKit Bug Importer]

<rdar://problem/73630008>

[Jonathan Bedard]

Created attachment 418472 [details]
Patch

[Jonathan Bedard]

Created attachment 418562 [details]
Patch

[Jonathan Bedard]

Created attachment 418599 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #4)
> Created attachment 418599 [details]
> Patch

The test failures caused by this change are because Mojave does not have Python 3. We’re about to update our Mac EWS bots, though.

[Jonathan Bedard]

Created attachment 418761 [details]
Patch

[Jonathan Bedard]

Created attachment 418889 [details]
Patch

[Jonathan Bedard]

Created attachment 418926 [details]
Patch

[Jonathan Bedard]

Created attachment 419015 [details]
Patch

[Jonathan Bedard]

Created attachment 419220 [details]
Patch

[Sam Sneddon [:gsnedders]]

Comment on attachment 419220 [details]
Patch

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

Non-reviewer review:

I'll point out that we have Python-using copies of most of these in wpt/xhr today, as both are ultimately derived from the same written-in-PHP-from-Opera-Software test suite. So starting with XHR might not be the most valuable place to start! That said, this mostly looks good.

> LayoutTests/http/tests/xmlhttprequest/null-auth.py:9
> +sys.stdout.write('Content-Type: text/html')
> +sys.stdout.write('\r\n\r\n')
> +
> +print('<p>Test that null values in XHR login/password parameters are treated correctly.</p>')
> +print('<p>No auth tokens should be sent with this request.</p>')

I find the combination of sys.stdout.write and print somewhat surprising, even though I understand _why_ it's doing it (to ensure headers are terminated with CRLF, and not caring about the body of the response). Equally, it feels like in many cases the print could be of a triple-quoted multi-line string, rather than like this. Not sure how difficult/worthwhile making such changes is, though?

> LayoutTests/http/tests/xmlhttprequest/resources/multipart-post-echo.py:16
> +for key in sorted(form.keys()):

That this is sorting them is a notable change from the original PHP, and causes changed expectations. I can't recall quite how precisely this is specified, but it would be nice to avoid changing the order? (Especially when CPython 3.6+ preserves order with dicts, so the iteration order isn't at the mercy of the dict construction any more.)

> LayoutTests/http/tests/local/formdata/send-form-data-constructed-from-form-using-open-panel-expected.txt:5
> -Response: submitted=true&checkbox=value2&radio=value3&text=value4&file=test.txt:text/plain:Hello
> +Response: checkbox=value2&radio=value3&submitted=true&text=value4&file=test.txt:text/plain:Hello

e.g., this change

[Jonathan Bedard]

Comment on attachment 419220 [details]
Patch

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

>> LayoutTests/http/tests/xmlhttprequest/null-auth.py:9
>> +print('<p>No auth tokens should be sent with this request.</p>')
> 
> I find the combination of sys.stdout.write and print somewhat surprising, even though I understand _why_ it's doing it (to ensure headers are terminated with CRLF, and not caring about the body of the response). Equally, it feels like in many cases the print could be of a triple-quoted multi-line string, rather than like this. Not sure how difficult/worthwhile making such changes is, though?

I'll upload a change that makes an attempt to standardize this...I think it's really going to be a question about style preferences.

>> LayoutTests/http/tests/xmlhttprequest/resources/multipart-post-echo.py:16
>> +for key in sorted(form.keys()):
> 
> That this is sorting them is a notable change from the original PHP, and causes changed expectations. I can't recall quite how precisely this is specified, but it would be nice to avoid changing the order? (Especially when CPython 3.6+ preserves order with dicts, so the iteration order isn't at the mercy of the dict construction any more.)

Turns out the cgi module pretty much always sorts this (ie, key in sorted(form.keys()), key in form.keys(), key in form are all sorted)

I'd sort of like to keep it sorted(form.keys()), so future contributors will immediately know how this is organized.

[Jonathan Bedard]

Created attachment 419442 [details]
Patch

[Jonathan Bedard]

Created attachment 419453 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #14)
> Created attachment 419453 [details]
> Patch

Not a big fan of how the multi-line strings end up looking....

[Alex Christensen]

Comment on attachment 419442 [details]
Patch

Why not perl?
r=me

[Jonathan Bedard]

(In reply to Alex Christensen from comment #16)
> Comment on attachment 419442 [details]
> Patch
> 
> Why not perl?
> r=me

Because there are many folks who want to replace our perl with Python :)

[Jonathan Bedard]

Created attachment 419608 [details]
Patch

[EWS]

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

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

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 419752 [details]
Patch

[EWS]

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

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