Problem/Motivation

Followup from #2294503: Component Utilities unit test cleanups. which had code cleanup for existing tests, and also added new tests. This is one of the new tests split out from that issue.

This issue adds unit tests for Drupal\Component\Utility\UserAgent.

The patch moves a test and some data from Drupal\language\Tests\LanguageBrowserDetectionUnitTest to UserAgentTest.

Note that this issue used to contain a test for Drupal\Component\Utility\Html, but that is now moved to another issue: #2383945: Expand test coverage for Drupal\Component\Utility\Html

Proposed resolution

  • Write the tests.
  • Evaulate the patch.
  • Commit the patch.
  • Yay.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only improves automated testing coverage.
CommentFileSizeAuthor
#5 interdiff.txt2.57 KBmile23
#5 2382011_5.patch10.72 KBmile23
#5 interdiff_3.txt3.02 KBmile23
#5 2382011_5.patch12.96 KBmile23
#3 interdiff.txt2.85 KBmile23
#3 2382011_3.patch13.29 KBmile23
#1 2382011_1.patch13.13 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2382011_1.patch13.13 KB

The patch.

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -0,0 +1,89 @@
+    $this->assertTrue(is_a($doc, '\DOMDocument'));

    Let's use $this->assertInstanceOf 

  2. +++ b/core/tests/Drupal/Tests/Component/Utility/UserAgentTest.php
@@ -0,0 +1,165 @@
+    $this->assertSame($expected, $result, $accept_language);

    Three parameters to assertSame?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2382011_3.patch13.29 KB
new interdiff.txt2.85 KB

1. Good catch. Done.

2. I suppose that's left over from the SimpleTest test, when a fail didn't show you the data set. Gone.

Found some formatting errors, and added a docblock for providerTestGetBestMatchingLangcode().

Also there should be a followup for this:

      // @todo: This is copied from RFC4647 but our regex skips the numbers so
      // they where removed. Our code should be updated so private1-private2 is
      // valid. http://tools.ietf.org/html/rfc4647#section-3.4
daffie’s picture

daffie commented
Status: Needs review » Needs work

I did a review and it looks good, but I have few remarks:
- The UserAgentTest is RTBC for me.
- The function providerTestLoad() in the HtmlTest class has no docblock.
- Nitpick: The function testLoad() in the HtmlTest class has only two parameters. The provider gives three parameters. Can we add the third parameter. I know it is not used.
- In the HtmlTest class there are no tests for the functions getClass() and getUniqueId(). There are more untested functions. But they are tested in other functions.

I know that we have problems with how html5 code in drupal. I think it would be a good idea to add a lot of extra test cases to the data provider of the HtmlTest class. Not now but in or after #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Expand unit testing for Drupal\Component\Utility classes (Html and UserAgent). » Expand unit testing for Drupal\Component\Utility\UserAgent
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2382011_5.patch12.96 KB
new interdiff_3.txt3.02 KB
new 2382011_5.patch10.72 KB
new interdiff.txt2.57 KB

OK, so for some reason no one moved core/tests/Drupal/Tests/Core/Utility/HtmlTest.php to the Component namespace. :-)

So I'll make another issue for HtmlTest, moving it to the proper namespace and adding the code from this issue.

In this patch: UserAgentTest and nothing else. :-)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Aw crap. Too many attached files.

Watch now as I fumble in the dark.

The latter set of patch+interdiff is the relevant one.

Patch: https://www.drupal.org/files/issues/2382011_5_0.patch

Interdiff: https://www.drupal.org/files/issues/interdiff_8307.txt

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes

New followup issue here: #2383945: Expand test coverage for Drupal\Component\Utility\Html

daffie’s picture

daffie commented

Double post

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

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

For the committer. The correct patch file is: 2382011_5_0.patch

@Mile23: No more working in the dark!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 4e90206 and pushed to 8.0.x. Thanks!

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

  • alexpott committed 4e90206 on 8.0.x 
    Issue #2382011 by Mile23: Expand unit testing for Drupal\Component\...

Status: Fixed » Closed (fixed)

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