Refactor UrlTest tests to use a dataProvider

I think the toString() will make a full URL, which is with http://etc. so it is the re-factoring that breaks it or the test was incorrect in the first place.

The data provider changed, because the original depended upon urls came from testUrlFromRequest, which are urls created by a path.

Those should still be fully qualified, so I think there are more bugs here ...

Here's a re-roll.

And a first step you might take:

+++ b/core/tests/Drupal/Tests/Core/UrlTest.php
@@ -373,13 +308,38 @@ protected function getMockAccessManager($access, $account = NULL) {
+  public function urlProvider() {
+    $data = [
+      ['view.frontpage.page_1', [], [], '/node'],
+      ['node_view', ['node' => '1'], [], '/node/1'],
+      ['node_edit', ['node' => '2'], [], '/node/2/edit'],
+    ];
+
+    // Create URL objects from data.
+    foreach ($data as &$value) {
+      array_unshift($value, Url::fromRoute($value[0], $value[1], $value[2]));
+    }
+
+    return $data;
   }

The problem here is that we are testing methods of the Url class, including fromRoute(). So if we create a Url object using fromRoute(), then we don't have any isolation of the method from other implementations.

Instead, we should generate a PHPUnit mocked object, using getMockBuilder(). Then you can specify what individual methods return and so forth, and keep everything isolated.

You can do this with, for instance, isExternal(). It's a non-static method which accesses a protected property of an Url instance. So you mock Url, then you use reflection (\ReflectionProperty) to set ::external to a known value. Then you can assert whether $mock->isExternal() returns that known value. For this kind of test you don't really need a dataprovider.

Unit testing is really code analysis, where you provide all the dependencies.

Retitling.

@Mile23 want to try that re-roll again? 2.5MB doesn't look re-rollish, your branch may not have been up-to-date with the latest 8.0.x

Hmm. That is a bit large. :-) Will check it out when I can.

Re-roll of #2. Ignore patch from #8, but hopefully not advice. :-)

@Mile23 thanks for the re-roll. Is the suggestion you made about using a getMockBuilder still needed for this patch? It's green and there were no changes/fixes in the re-roll.

Maybe it's good to go as-is?

@hctom what are your thoughts on the getMockBuilder suggestion in #8?

Generally a mock builder is a good thing to have, but we are not as strict in core about unit vs. integration testing right now.

So we have several unit tests that are really integration tests.

Though typically those tests using data providers / Fixtures are often the integration tests.

Fascinating that tests now pass ...

Do you feel lucky testbot?... well do ya?

@hctom, sorry. I was on vacation and got busy with work when I got back. I will look into this tomorrow to see if my original issues from #2350507: \Drupal\Core\Url has no __toString() magic method are still relevant. Thanks!

The stuff about mocking really comes down to the question: Is it a unit test if it's not isolated? In Drupal-land it apparently is.

If someone wants to RTBC I won't complain. :-)

This is an integration test, but as Drupal is very used to integration tests and it was an integration test before, I am okay this being kept an integration test that uses a data provider.

Therefore: RTBC

Re-classifying as a bug report due to:

... we realized that the test methods of \Drupal\Tests\Core\UrlTest interfere with each other, which makes reliable tests impossible.

Which is a bug as our tests should be reliable.

+1

+++ b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
@@ -117,105 +109,119 @@ public function testFromUriWithNonScheme() {
-   * @depends testFromUri
+   * @dataProvider urlProvider

While @depends is certainly nice for some usecases like moving state from one test to the next one, its not a good idea for sharing test data. @dataProvider is also much easier to understand.

  /**
   * Tests the __toString() method.
   *
   * @param \Drupal\Core\Url[] $urls
   *   An array of Url objects.
   *
   * @depends testUrlFromRequest
   *
   * @covers ::__toString
   */
  public function testMagicToString($urls) {
    foreach ($urls as $index => $url) {
      $url->setUrlGenerator(\Drupal::urlGenerator());
      $path = array_pop($this->map[$index]);
      $this->assertSame($path, (string) $url);
    }
  }

This needs to be updated... from \Drupal\Tests\Core\UrlTest

Reroll of #26.

You can run individual PHPUnit tests like this:

$ cd core
$ ./vendor/bin/phpunit --filter=YourTestClassHere

In UnroutedUrlTest, setUp() must be public.

When I set them public I get a bunch of errors like this:

[04-Dec-2014 21:37:05 UTC] PHP Warning:  parse_url() expects parameter 1 to be string, array given in /Users/paulmitchum/projects/drupal8/core/lib/Drupal/Core/Url.php on line 228

[04-Dec-2014 21:37:05 UTC] PHP Warning:  htmlspecialchars() expects parameter 1 to be string, array given in /Users/paulmitchum/projects/drupal8/core/lib/Drupal/Component/Utility/String.php on line 36

UrlTest has the same problem with setUp() not being public and throwing the same errors. It also gives me this:

$ ./vendor/bin/phpunit --filter=UrlTest
PHPUnit 4.1.4 by Sebastian Bergmann.

Configuration read from /Users/paulmitchum/projects/drupal8/core/phpunit.xml.dist

..........................EFFFEEE..........................

Time: 5.52 seconds, Memory: 105.25Mb

There were 4 errors:

1) Drupal\Tests\Core\UrlTest::testGetInternalPath
Invalid argument supplied for foreach()

/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/Core/UrlTest.php:198

Do you have any idea why the setup() method has to be public? When I look at other tests in core 90% have protected setUp() methods. Only a small list of tests has public setUp() methods.

Every PHPUnit-based test as a public setUp() method. It's like that because the test runner has to be able to run setUp().

@Mile23: Unfortunately you did not really applied all my changes in the reroll of the patch.

It was a straight reroll.

Applied patch in #31.

Ran UrlTest:

$ ./vendor/bin/phpunit --filter=UrlTest
PHPUnit 4.1.4 by Sebastian Bergmann.

Configuration read from /Users/paulmitchum/projects/drupal8/core/phpunit.xml.dist

.................................................FFFEEE.......... 65 / 81 ( 80%)
................

Time: 5.81 seconds, Memory: 105.25Mb

There were 3 errors:
$

Which is to say error reporting crashed, with this error:
PHP Catchable fatal error: Method Drupal\Core\Url::__toString() must return a string value in [..]core/vendor/phpunit/phpunit/src/Framework/TestCase.php on line 1707

This leads me to believe that PHPUnit is having trouble presenting the data set, trying to show you what it is, but failing because _toString() doesn't work for some reason. That reason is likely that its Url object has fallen out of scope, or is set NULL for testing. This might be a bug in PHPUnit, but I doubt it.

Changing Url so it doesn't have a _toString() method any more (and commenting out testMagicToString()), I get some useful results:

There were 3 failures:

1) Drupal\Tests\Core\UrlTest::testToString with data set #0 (Drupal\Core\Url, 'view.frontpage.page_1', array(), array(), '/node')
Failed asserting that null is identical to '/node'.

/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/Core/UrlTest.php:201

2) Drupal\Tests\Core\UrlTest::testToString with data set #1 (Drupal\Core\Url, 'node_view', array('1'), array(), '/node/1')
Failed asserting that null is identical to '/node/1'.

/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/Core/UrlTest.php:201

3) Drupal\Tests\Core\UrlTest::testToString with data set #2 (Drupal\Core\Url, 'node_edit', array('2'), array(), '/node/2/edit')
Failed asserting that null is identical to '/node/2/edit'.

/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/Core/UrlTest.php:201

So maybe those are some clues.

This is a PHPUnitTest. And in this case that means that we are testing the class \Drupal\Core\Url. And that is the only class that should be tested. No other classes. That means that mocks are needed. Lets start with Url::fromUri in the function providerUrl().

Much of the patch here is outdated. All the changes to UnroutedUrlTest.php have been done. What remains is to refactor UrlTest.php, which on a brief look just looks like it is being converted to use a dataprovider. I don't see anything here that makes this a bug so changing to a Task and updating the title.