Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
6.67 KB

Here's a start that brings coverage from 48% to 68%. The remaining uncovered code seems to be largely dependent on specific environments from what I can tell.

dawehner’s picture

As always, what a impressive amount of detail!

So which code is hard to test:

  public static function check() {
    // Check for mbstring extension.
    if (!function_exists('mb_strlen')) {
      static::$status = static::STATUS_SINGLEBYTE;
      return 'mb_strlen';
    }

    // Check mbstring configuration.
    if (ini_get('mbstring.func_overload') != 0) {
      static::$status = static::STATUS_ERROR;
      return 'mbstring.func_overload';
    }
    if (ini_get('mbstring.encoding_translation') != 0) {
      static::$status = static::STATUS_ERROR;
      return 'mbstring.encoding_translation';
    }
    if (ini_get('mbstring.http_input') != 'pass') {
      static::$status = static::STATUS_ERROR;
      return 'mbstring.http_input';
    }
    if (ini_get('mbstring.http_output') != 'pass') {
      static::$status = static::STATUS_ERROR;
      return 'mbstring.http_output';
    }

    // Set appropriate configuration.
    mb_internal_encoding('utf-8');
    mb_language('uni');
    static::$status = static::STATUS_MULTIBYTE;
    return '';
  }


  /**
   * Converts data to UTF-8.
   *
   * Requires the iconv, GNU recode or mbstring PHP extension.
   *
   * @param string $data
   *   The data to be converted.
   * @param string $encoding
   *   The encoding that the data is in.
   *
   * @return string|bool
   *   Converted data or FALSE.
   */
  public static function convertToUtf8($data, $encoding) {
    if (function_exists('iconv')) {
      return @iconv($encoding, 'utf-8', $data);
    }
    elseif (function_exists('mb_convert_encoding')) {
      return @mb_convert_encoding($data, 'utf-8', $encoding);
    }
    elseif (function_exists('recode_string')) {
      return @recode_string($encoding . '..utf-8', $data);
    }
    // Cannot convert.
    return FALSE;
  }

as well as large amount of substr (which maybe uses also external code if possible)

At least for substr() we can call setStatus directly, what do you think?

dawehner’s picture

Status: Needs review » Needs work
Mile23’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
7.63 KB

Added single- and multi-byte code paths to substr and strlen tests.

It'd require some refactoring to test check() and covertToUtf8() adequately, which might slow things down.

Still though... C.R.A.P. score went from 788 to 80. :-)

ParisLiakos’s picture

  1. +++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
    @@ -12,6 +12,8 @@
    + *
    + * @see \Drupal\Component\Utility\Unicode
      */
     class UnicodeTest extends UnitTestCase {
    

    Lets also add @group Drupal too? and maybe unicode?

  2. +++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
    @@ -82,9 +84,11 @@ public function testMimeHeader($value, $encoded) {
    -  public function providerTestMimeHeader() {
    +  public static function providerTestMimeHeader() {
    

    i can see why we static here, but maybe we should add it on #2057905: [policy, no patch] Discuss the standards for phpunit based tests so other people are aware? Right now only one provider is static in core

Mile23’s picture

I hadn't noticed that the data providers were set static. I'm curious why that would be preferable (or not).

dawehner’s picture

Let's not make them static, as there is no point in doing it.

Mile23’s picture

Took away the static.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Really impressive additional test coverage!

+++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
@@ -12,6 +12,8 @@
 /**
  * Test unicode handling features implemented in Unicode component.
+ *
+ * @see \Drupal\Component\Utility\Unicode
  */
 class UnicodeTest extends UnitTestCase {

Just in case you have to do another rerole you could add @group Drupal and @group Unicode

Mile23’s picture

Doh!

Will add when the test comes back.

Mile23’s picture

Added @group Drupal and Unicode.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, great stuff, thanks!

Committed and pushed to 8.x.

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

jhedstrom’s picture