Expand test coverage for Drupal\Component\Utility\Html

I have two remarks for this patch.

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -0,0 +1,278 @@
+    $property = new \ReflectionProperty('Drupal\Component\Utility\Html', 'seenIdsInit');
+    $property->setAccessible(TRUE);
+    $property->setValue(NULL);

Probably a stupid question: Why do we need this? The variable $property is local?

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -0,0 +1,278 @@
+  public function testHtmlClass() {

Why is this function not called testGetClass? The same for the other functions?

@Mile23: I know what reflections are. Mostly. But my remark was that you put it in a local function variable. As far as I know you cannot use that variable outside of the function. Or am I wrong?

@Mile23: You only changed testHtmlClass(). There are more testHtml* functions.

@Mile23: Thank you for explaining. Again I learned something new.

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -0,0 +1,279 @@
+   * Provides test data for testHtmlGetId().
...
+   * Provides test data for testHtmlGetId().
...
+   * Provides test data for testHtmlGetId().

The function name has changed to testGetId()

Some minor comment remarks.

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -0,0 +1,257 @@
+  /**
+   * Tests the Html::getUniqueId() method.
+   *
+   * @dataProvider providerTestGetUniqueIdWithAjaxIds
+   * @covers ::getUniqueId
+   */
+  public function testGetUniqueIdWithAjaxIds($expected, $source, $reset = FALSE) {

The first line: We are testing Html::getUniqueIdWithAjaxIds().

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -0,0 +1,257 @@
+  /**
+   * Tests the Html::getUniqueId() method.
+   *
+   * @dataProvider providerTestGetId
+   * @covers ::getId
+   */
+  public function testGetId($expected, $source) {

The first line: We are testing Html::getId().

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -0,0 +1,257 @@
+   * Test HTML document loading.
...
+   * Test HTML document serialization.
...
+   * Test HTML normalization.

In the rest of the class it is: "Tests the Html::theTestMethod() method."

The HtmlTest has been moved and has been extended.
The old test file has been deleted.
The comments are all in order.
It looks good to me so I give it a RTBC.

Not downgrading but in these cases it really helps to roll patches with -C -M in order to allow for easier reviewing.

The serialise test in giving a false impression of test coverage.

  public static function serialize(\DOMDocument $document) {
    $body_node = $document->getElementsByTagName('body')->item(0);
    $html = '';

    foreach ($body_node->getElementsByTagName('script') as $node) {
      static::escapeCdataElement($node);
    }
    foreach ($body_node->getElementsByTagName('style') as $node) {
      static::escapeCdataElement($node, '/*', '*/');
    }
    foreach ($body_node->childNodes as $node) {
      $html .= $document->saveXML($node);
    }
    return $html;
  }

We should be passing in some script and styles to test that that code has the expected affect.

Also can the move and test additions and fixes happen in separate issues.

 * Tests \Drupal\Component\Utility\Html.
 *

This is already covered by @coversDefaultClass.

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -67,47 +60,42 @@ public function providerTestCleanCssIdentifier() {
    * Tests that Html::getClass() cleans the class name properly.
    *

Can we remove this. We already have @covers ::getClass.

Can we rename providerTestCleanCssIdentifier() to providerCleanCssIdentifier().

Can we rename providerTestGetUniqueId() to providerGetUniqueId().

Can we rename providerTestGetUniqueIdWithAjaxIds() to providerGetUniqueIdWithAjaxIds().

Can we rename providerTestGetId() to providerGetId().

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -210,4 +186,58 @@ public function providerTestHtmlGetId() {
+  public function providerTestLoad() {

Can we rename this provider function. It is used multiple times. So it can become providerLoad() or providerHtml().

In the function testLoad we do not have an assertion like:

$this->assertEquals($expected, $doc);

Why not? If we add such an assertion, can we make $expected the first parameter in the testLoad() method.

Still needs work

I addressed each of @daffie's comments except for this guy:

In the function testLoad we do not have an assertion like:

$this->assertEquals($expected, $doc);
Why not? If we add such an assertion, can we make $expected the first parameter in the testLoad() method.

It looks like we're analyzing $doc by looking at its tags. Are you suggesting asserting that the entire doc is equal to something expected? Do you mind clarifying?

Whoops, forgot my interdiff.

Now reviewing.

I looked at the full patch, looks good.

I looked at the interdiff to see if it matched all of daffie's comments from #24 at it looks good.

Regarding daffie's comment about testLoad, it seems to me that the tests that are there for testLoad are sufficient for merging. If someone wanted to expand on them more I suggest a follow up.

Marking as RTCB.

1. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
   @@ -25,24 +23,14 @@ class HtmlTest extends UnitTestCase {
   -    $property = new \ReflectionProperty('Drupal\Component\Utility\Html', 'seenIdsInit');
   -    $property->setAccessible(TRUE);
   -    $property->setValue(NULL);
   ...
   +    $ref_seen_ids_init = new \ReflectionProperty('Drupal\Component\Utility\Html', 'seenIdsInit');
   +    $ref_seen_ids_init->setAccessible(TRUE);
   +    $ref_seen_ids_init->setValue(NULL);
   

   This doesn't actually change anything, just adding to the patch size.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
   @@ -210,4 +182,58 @@ public function providerTestHtmlGetId() {
   +    return array(
   

   Here and elsewhere, use [] for new code.

The issue summary needs an update because we no longer move code around.

@tim.plunkett if they change all the new stuff to [] then this file will be half new half old... so I don't think that's really worth it - as far as I remember the current agreement is each file should be internally consistent. I'm not particularly bothered by #32.1 - fine either way by me - although in general I side on the lease change to achieve the task in the issue summary - which in this case is to add more test coverage and therefore in reality this change is out of scope.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

This one actually appears to still be relevant