Component Utilities unit test cleanups.

Potentially overlapping issue here: #2298667: Code review for report generation

Coming here from #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases. I wonder if this is the source of the below error that we're trying to address in this issue?

$ phpunit -c core/phpunit.xml.dist --coverage-html /tmp/report
PHPUnit 4.1.3 by Sebastian Bergmann.

Configuration read from /var/www/html/head/core/phpunit.xml.dist

..............................................   61 / 5258 (  1%)
...............................................  122 / 5258 (  2%)
...............................................  183 / 5258 (  3%)
...............................................  244 / 5258 (  4%)
...............................................  305 / 5258 (  5%)
...............................................  366 / 5258 (  6%)
...............................................  427 / 5258 (  8%)
.......Trying to @cover or @use not existing method "\Drupal\Component\Utility\Url::externalIsLocal".

FYI, 2 days ago it was not failing that early (but failing, still). I don't see anything else in the commits but #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases. that would have changed anything.

Yah, it looks like a bad @coversDefaultClass made it through #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases., but the code and the test are good.

The patch at the top no longer applies.

1. +++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
   @@ -20,6 +23,8 @@ class SortArrayTest extends UnitTestCase {
       * @dataProvider providerSortByWeightElement
   +   * @covers ::sortByWeightElement
   +   * @covers ::sortByKeyInt
   
   @@ -98,6 +99,8 @@ public function providerSortByWeightElement() {
       * @dataProvider providerSortByWeightProperty
   +   * @covers ::sortByWeightProperty
   +   * @covers ::sortByKeyInt
   
   @@ -176,6 +175,8 @@ public function providerSortByWeightProperty() {
       * @dataProvider providerSortByTitleElement
   +   * @covers ::sortByTitleElement
   +   * @covers ::sortByKeyString
   
   @@ -247,6 +244,8 @@ public function providerSortByTitleElement() {
       * @dataProvider providerSortByTitleProperty
   +   * @covers ::sortByTitleProperty
   +   * @covers ::sortByKeyString
   
   @@ -318,6 +313,9 @@ public function providerSortByTitleProperty() {
       * @dataProvider providerTestSortByWeightAndTitleKey
   +   * @covers ::sortByWeightAndTitleKey
   +   * @covers ::sortByKeyString
   +   * @covers ::sortByKeyInt
   

   It looks like these tests are double- and triple-dipping for coverage. The test only directly calls the first one in all of those cases, and there aren't any tests for some of the other ones.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/TimerTest.php
   @@ -11,15 +11,20 @@
   -   * @see \Drupal\Component\Utility\Timer::read()
   +   * @covers ::start
   +   * @covers ::stop
   +   * @covers ::read
   

   This is a functional test so it doesn't cover anything. Should be marked @coversNothing.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
   @@ -11,20 +11,30 @@
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * @covers ::check
   +   */
      public function setUp() {
        // Initialize unicode component.
        Unicode::check();
      }
   

   Can a setUp() really be considered to cover some code?

Some other @covers beyond that.

I don't understand why this patch performs the following change everywhere:

   /**
    * Tests external versus internal paths.
    *
+   * @dataProvider providerTestIsExternal
+   * @covers ::isExternal
+   *
    * @param string $path
...
-   *
-   * @dataProvider providerTestIsExternal
-   * @covers ::isExternal
    */
   public function testIsExternal($path, $expected) {

Can we keep the functional method annotations closer to the method definition, please? (i.e., leave them were they are)

Especially a specified @dataProvider should be as quickly visible as possible. By moving it up, you first have to scroll away that other, overly verbose PHPDoc garbage.

Just to note that today I'm seeing another failure, a bit later in the tests.

$ phpunit -c core/phpunit.xml.dist --coverage-html /tmp/report
PHPUnit 4.1.3 by Sebastian Bergmann.

Configuration read from /var/www/html/head/core/phpunit.xml.dist

.............................................  427 / 5323 (  8%)
.............................................  488 / 5323 (  9%)
.............................................  549 / 5323 ( 10%)
.............................................  610 / 5323 ( 11%)
.............................................  671 / 5323 ( 12%)
.....Trying to @cover or @use not existing method "\Drupal\Core\Asset\LibraryDiscoveryParser::buildLibrariesByExtension".

Looks related to #2276219: Asset libraries should declare their license

@anvarre: That's a bit out of scope for this issue, but it's true that if we're using @covers (and we are), then the @covers annotation should at least be accurate. :-) Also, generating a coverage report should be part of the continuous integration process so these errors are caught up-front.

AFAIK, that issue is #2253915: [policy, no patch] Fix coding standards for PHPUnit tests

As also noted over there, I strongly disagree with applying our overly verbose PHPDoc standards to PHPUnit tests/methods. IMO, @param and @return shouldn't even exist; all unit test code should (or actually MUST) be of trivial complexity; if you need to explain anything, then your test has a fundamental problem. The vast majority of unit test methods should not have any PHPDoc at all; most of the rest should be happy with a single @dataProvider annotation, nothing else.

Your thinking of standardization/harmonization is very appreciated (because quality), but yeah, I think you've picked and applied/adopted the wrong baseline here. It would be great if we can undo those changes and defer the standards discussion to #2253915.

Re-roll.

And this should work.

The main problem was UserAgentTest, which contained a reference to \Drupal\Core\Language\Language, and the deprecated randomName().

I refactored the setUp() method into two methods which supply language codes and mappings. Using $this->someProperty for dependencies in unit tests is generally bad form.

@neclimdul: I'd split this out into two more issues, one for UserAgentTest and one for HtmlTest, leaving the actual cleanup part here.

I'd do it but it's really your code. :-)

Ideally this would have been separate issues, but I don't think that's crucial.

Added a beta evaluation.

Here's the cleanup without the extra tests (UserAgentTest and HtmlTest) or changes to phpunit.xml.dist.

Note that I removed the UserAgent test and put it back into LanguageBrowserDetectionUnitTest.

Followups to follow. :-)

The new tests (UserAgentTest, HtmlTest) live here now: #2382011: Expand unit testing for Drupal\Component\Utility\UserAgent

#21 is good work towards standardizing how the phpunit tests are written, and is allowed under beta evaluation.

Committed 65f1d49 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.