Unit test the \Drupal class

+  /**
+   * Tests the flook() method.
+   */
+  public function testFlood() {

flook => flood

Mh where is the other issue which had also a unit test for Drupal, posted probably yesterday.

1. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -0,0 +1,336 @@
   +      ->with($this->equalTo('test_config'));
   ...
   +      ->with($this->equalTo('test_queue'), $this->equalTo(TRUE));
   ...
   +      ->with($this->equalTo('test_collection'));
   ...
   +      ->with($this->equalTo('test_entity'), $this->equalTo('OR'));
   ...
   +      ->with($this->equalTo('test_entity'), $this->equalTo('OR'));
   ...
   +      ->with($this->equalTo('test_route'), $this->equalTo($test_array), $this->equalTo($test_array));
   ...
   +      ->with($this->equalTo('Test title'), $this->equalTo('test_route'), $this->equalTo($test_array), $this->equalTo($test_array));
   ...
   +      ->with($this->equalTo($service_name));
   

   You are so not lazy: ... this could be just ->with($service_name) ...

2. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -0,0 +1,336 @@
   +    $test_array = array('test' => 'test');
   ...
   +    \Drupal::url('test_route', $test_array, $test_array);
   ...
   +    $test_array = array('test' => 'test');
   ...
   +    \Drupal::l('Test title', 'test_route', $test_array, $test_array);
   

   Any reasons to not have two different arrays and better namings

Perfect, thank you!

I'd suggest we'll eventually want strict in phpunit.xml.dist, so we should throw explicit assertions, even just assertNotNull().

May be one for the coding standards: #2057905: [policy, no patch] Discuss the standards for phpunit based tests

There are a bunch of other tests with no assertions, so whatever problem this might be is already present and we're better off with the not-strict test than without, so don't let me kill the RTBC.

Applies and passes here.

I was concerned that testUrl() and testL() might need dataproviders, but I see that we're testing the round trip through \Drupal, not the services themselves.

Just wondering whether we should check the interfaces at least in places where we know that an interface exists?

I've got to admit I do not get the point of this test. The point of a unit test is to test behaviour - if there is no behaviour in the method then why test - see http://stackoverflow.com/questions/6197370/should-unit-tests-be-written-...

Leaving at rtbc for more opinions.

Also the issue with #2078285: Add short-cut methods to the \Drupal class for generating URLs and links from routes is that we should not add methods with uses.

The thing is that some of them just had bugs, which hopefully would be caught in some client test code, but you never know.

Here's why you want a unit test of \Drupal:

  /**
   * Returns an expirable key value store collection.
   *
   * @param string $collection
   *   The name of the collection holding key and value pairs.
   *
   * @return \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface
   *   An expirable key value store collection.
   */
  public static function keyValueExpirable($collection) {
    return static::$container->get('keyvalue.expirable')->get($collection);
  }

Add a typo so it says return static::$container->get('key_value.expirable')->get($collection); and run PHPUnit.

(Hint: It passes, because there are no unit tests.)

Much more convenient to run an 8-second test than all of SimpleTest to discover this typo.

Here's a version that mocks interfaces instead of implementations and has more specific demands on return values, for testL() and testUrl(). Other classes used don't implement appropriate interfaces, so we stick with the implementation class for those and just leave assertNotNull.

17: 2089787-17.patch queued for re-testing.

Changed typedData() test to typedDataManager().

Can we just add a testcase for formBuilder to complete it? Otherwise this is RTBC

Oo. Lookit that, someone added another method.

Done. :-)

RTBC as per #22.

I actually find the argument in #17 pretty persuasive in why a simple check here makes sense. I'd rather not do these kinds of smokescreen tests to all classes, but the Drupal class is central enough that it'd be nice to pick up issues introduced in it sooner than after commit.

So... Committed and pushed to 8.x. Thanks!

...Plus it just looks nice. :-)