ext/uri: Reorganises legacy tests - #23286
Conversation
| } | ||
| } catch (Throwable $e) { | ||
| echo $e::class, ": ", $e->getMessage(), PHP_EOL; | ||
| echo $e::class, ': ', $e->getMessage(), "\n"; |
There was a problem hiding this comment.
Since the file now is in git history anyway (move), I unified the style. Cool?
| Uri\WhatWg\Url::parse(" https://example.org ", errors: $f->x); | ||
| } catch (Throwable $e) { | ||
| echo $e::class, ": ", $e->getMessage(), PHP_EOL; | ||
| echo $e::class, ': ', $e->getMessage(), "\n"; |
There was a problem hiding this comment.
Two options for these: keep grouped in eg parsing with zend_ prefix or give dedicated zend dir. Thoughts?
74844f2 to
d2bfb5c
Compare
kocsismate
left a comment
kocsismate
left a comment
There was a problem hiding this comment.
partial review yet, I'll continue later
| @@ -1,5 +1,5 @@ | |||
| --TEST-- | |||
| Test Uri getters | |||
There was a problem hiding this comment.
these cases are already covered by the individual getter tests, aren't they?
| <?php | ||
|
|
||
| $uri1 = Uri\Rfc3986\Uri::parse("https://example.com"); | ||
| $uri2 = $uri1->withHost(null); |
There was a problem hiding this comment.
Why is the host unset here first? And there is no assertion for it. 🤔
I also found https://github.com/php/php-src/blob/65ad51c2faf16d40b0b7b916bfbaba2468ab3f95/ext/uri/tests/rfc3986/modification/host_success_ipv4.phpt and https://github.com/php/php-src/blob/65ad51c2faf16d40b0b7b916bfbaba2468ab3f95/ext/uri/tests/rfc3986/modification/host_success_ipv6.phpt which tests very similar things. So I'm wondering if we need this test in the first place? Does it assert something those tests don't?
| <?php | ||
|
|
||
| $uri1 = Uri\Rfc3986\Uri::parse("https://example.com"); | ||
| $uri1->withHost("t%3As%2Ft.com"); |
There was a problem hiding this comment.
The assertion is missing from here
| $uri2 = $uri1->withPort(80); | ||
|
|
||
| var_dump($uri1->getPort()); | ||
| var_dump($uri2->getPort()); |
There was a problem hiding this comment.
modification tests also assert the full URI, like here:
var_dump($uri1->getPort());
var_dump($uri2->getPort());
var_dump($uri2->toRawString());
| $uri1 = Uri\Rfc3986\Uri::parse("/foo"); | ||
| $uri2 = $uri1->withPort(null); | ||
|
|
||
| var_dump($uri2->getPort()); |
There was a problem hiding this comment.
$uri1->getPort() and the full URI of $uri2 should be asserted as well
| @@ -1,17 +1,16 @@ | |||
| --TEST-- | |||
| Test percent-encoding normalization - special case | |||
| Test Uri\Rfc3986\Uri parsing - path - percent-encoded reserved character | |||
There was a problem hiding this comment.
| Test Uri\Rfc3986\Uri parsing - path - percent-encoded reserved character | |
| Test Uri\Rfc3986\Uri parsing - path - percent-encoded slash character |
|
|
||
| $url = new Uri\WhatWg\Url("https://example.com/foo/bar%2Fbaz"); | ||
| var_dump($url->getPath()); | ||
| var_dump($uri->toRawString()); |
There was a problem hiding this comment.
tests in the parsing directory have the following code for assertions:
var_dump($uri);
var_dump($uri->toRawString());
var_dump($uri->toString());
```
|
|
||
| $uri = Uri\Rfc3986\Uri::parse("192.168/contact.html"); | ||
|
|
||
| var_dump($uri); |
There was a problem hiding this comment.
the same assertions are needed as mentioned above
| --FILE-- | ||
| <?php | ||
|
|
||
| $uri = new Uri\Rfc3986\Uri("mailto:johndoe@example.com"); |
There was a problem hiding this comment.
much of these cases are already covered, please check them (e.g. there is ext/uri/tests/rfc3986/parsing/path_success_relative_reference.phpt and ext/uri/tests/rfc3986/parsing/path_success_relative_reference_absolute.phpt). so probably only mailto:johndoe@example.com and maybe /// is needed (but let's separate them for better readability)
There was a problem hiding this comment.
"of" is missing from the test title
To avoid miscommunication: I plan to address everything in one go when you continued. In case your second pass didn't yield anything new please update me so I know it's done and we do not wait for each other. :) |
2 participants
Ref: #23271 (review) @kocsismate
Depends on: #23271 (
ready here, but marked as draft until 23271 is merged)Coverage