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

See #1938068: 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

jhedstrom’s picture

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.

jhedstrom’s picture

Issue tags:+phpunit

Adding missing phpunit tag.

jhedstrom’s picture

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().

dawehner’s picture

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.

jhedstrom’s picture

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/'

dawehner’s picture

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.

Mile23’s picture

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.

dawehner’s picture

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.

Mile23’s picture

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.

Mile23’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Mile23’s picture

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.

Mile23’s picture

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.

dawehner’s picture

+++ 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.

Mile23’s picture

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

ParisLiakos’s picture

Status:Needs review» Needs work

yes besides this small docs issue i cant see anything else

jhedstrom’s picture

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.

Mile23’s picture

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

CRAP 163 to 32.

Reviewed and Tested By CRAP.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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

jhedstrom’s picture