Task to expand test coverage for \Drupal\Component\Utility\Url.

See #1938068: [Meta] Convert UnitTestBase to PHPUnit.

Files: 
CommentFileSizeAuthor
#18 Screen Shot 2013-10-10 at 11.21.41 PM.png51.7 KBMile23
#18 Screen Shot 2013-10-10 at 11.29.59 PM.png50.81 KBMile23
#17 url-phpunit-2046245-17.patch8.24 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 58,846 pass(es).
[ View ]
#17 interdiff.txt716 bytesjhedstrom
#13 url-phpunit-2046245-13.patch8.06 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]
#9 url-phpunit-2046245-9.patch7.15 KBMile23
FAILED: [[SimpleTest]]: [MySQL] 58,583 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#7 url-phpunit-2046245-7.patch7.84 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 58,486 pass(es).
[ View ]
#7 url-phpunit-2046245-7.interdiff.txt2.04 KBMile23
#6 url-phpunit-2046245-6.patch7.26 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,541 pass(es).
[ View ]
#6 interdiff.txt1.32 KBdawehner
#3 url-phpunit-2046245-03.patch6.2 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 57,221 pass(es).
[ View ]
#1 failing.txt1.18 KBjhedstrom
#1 url-phpunit-2046245-01.patch8.4 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 57,304 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,304 pass(es).
[ View ]
new1.18 KB

This patch ups coverage to 93.8%. Currently not sure why tests for externalIsLocal are failing, but I've attached those as a diff, as perhaps I'm calling it incorrectly, or perhaps it is legitimately broken.

Issue tags:+phpunit

Adding missing phpunit tag.

StatusFileSize
new6.2 KB
PASSED: [[SimpleTest]]: [MySQL] 57,221 pass(es).
[ View ]

Oops, #1 had some code from a different patch. The failing.txt file there is still relevant fo externalIsLocal().

Wow, we are getting better and better on our test coverage!

There is a missing test for externalIsLocal() if we want to cover them all.

Re: #4 see the failing.txt patch in #1. I'm either mis-calling externalIsLocal(), or it is mis-documented, or it is broken. Specifically, calling with these parameters does not return true:

'http://example.com/internal/path', 'http://example.com/'

StatusFileSize
new1.32 KB
new7.26 KB
PASSED: [[SimpleTest]]: [MySQL] 57,541 pass(es).
[ View ]

Yeah this function was never tested, as it just got introduced into Drupal 8.

While running these tests I get a lot of messages which looks like:

Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.

StatusFileSize
new2.04 KB
new7.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,486 pass(es).
[ View ]

Added a test for when url and base_url are the same in externalIsLocal().

In the process I found a bug. :-) Actually it still had the same result but it never called line 232, so never benefitted from the short-circuit. Reading up on parse_url(), it seems the path will be '/' if the domain name has a trailing '/'.

This is one reason that 100% coverage isn't such a bad idea: You find bugs.

Status:Needs review» Needs work

In the process I found a bug. :-) Actually it still had the same result but it never called line 232, so never benefitted from the short-circuit. Reading up on parse_url(), it seems the path will be '/' if the domain name has a trailing '/'.

That is why we need this intensive unit tests!

+++ b/core/tests/Drupal/Tests/Component/Utility/UrlTest.php
@@ -189,6 +190,185 @@ public function testInvalidRelative($url, $prefix) {
+  public function testFilterQueryParameters($query, $exclude, $expected) {
...
+   * Provides data to self::testFilterQueryParameters().
+   */
+  public static function providerTestFilterQueryParameters() {
...
+  public function testParse($url, $expected) {
...
+  public static function providerTestParse() {
...
+  public function testEncodePath($path, $expected) {
...
+  public static function providerTestEncodePath() {
...
+  public function testIsExternal($path, $expected) {
...
+   */
+  public static function providerTestIsExternal() {
...
+  public function testFilterBadProtocol($uri, $expected, $protocols) {
...
+   */
+  public static function providerTestFilterBadProtocol() {
...
+   */
+  public function testStripDangerousProtocols($uri, $expected, $protocols) {
...
+   */
+  public static function providerTestStripDangerousProtocols() {
@@ -228,4 +408,29 @@ protected function dataEnhanceWithPrefix(array $urls) {
+   */
+  public function providerTestExternalIsLocal() {
...
+   */
+  public function testExternalIsLocal($url, $base_url, $expected) {

Let's add parameter documentation / return statements.

StatusFileSize
new7.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,583 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

The same as #7, minus the bug fix, which now has its own issue: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, url-phpunit-2046245-9.patch, failed testing.

OK, feel kind of stupid here. :-)

Just go back to #6, and I'll put my test additions on the other issue: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.

Status:Needs work» Needs review
StatusFileSize
new8.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]

Started with #6.

@param and @return added.

Removed the test for externalIsLocal(), which will be covered by the other issue.

I think @dataProvider annotation should come last in the docblock, so I put them there. No guidance (yet) from documentation standards.

+++ b/core/tests/Drupal/Tests/Component/Utility/UrlTest.php
@@ -189,6 +198,230 @@ public function testInvalidRelative($url, $prefix) {
+   * @param array $query
+   * @param array $exclude
+   * @param array $expected

The actual point in documentation is to explain what these parameters are used for, not to just list the parameters. This is information i can get by just looking at the code.

I think I left those alone because I didn't understand what was being tested. The rest of the docblocks look better.

Status:Needs review» Needs work

yes besides this small docs issue i cant see anything else

Status:Needs work» Needs review
StatusFileSize
new716 bytes
new8.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,846 pass(es).
[ View ]

Fixes the docblock issues from #14.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new50.81 KB
new51.7 KB

CRAP 163 to 32.

Reviewed and Tested By CRAP.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.