Expand and convert to phpunit tests for \Drupal\Component\Transliteration

+++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
@@ -0,0 +1,107 @@
+    $printable = (isset($case[3])) ? $case[3] : $original;

so when $case is ever set here?

Let's remove the need for a transliteration_test module.

Yeah, this removes a webtest!

• Introduces the module handler on the core class and replaces the drupal_alter() call
• Adds a core specific test, which takes care of the alter functionality.
• Removes the transliteration_test module

I do agree that the interdiff is more confusing as helpful.

There we go.

Yay for getting rid a test module:D

1. +++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
   @@ -2,24 +2,23 @@
   + * Contains Drupal\Tests\Component\Transliteration\PhpTransliterationTest.
   

   Drupal needs a backslash in front here.. \Drupal\..

2. +++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
   @@ -31,9 +30,40 @@ public static function getInfo() {
   +   * @dataProvider providerTestPHPTransliteration
   ...
   +   * Provides data for self::testPHPTransliteration().
   ...
   +  public static function providerTestPHPTransliteration() {
   
   +++ b/core/tests/Drupal/Tests/Core/Transliteration/PHPTransliterationTest.php
   @@ -0,0 +1,105 @@
   +  public function testPHPTransliterationWithAlter($langcode, $original, $expected, $printable = NULL) {
   ...
   +  public function providerTestPHPTransliterationWithAlter() {
   

   PHP should be only uppercase first ie: testPhpTransliteration

well we should at least name correctly the newly introduced code here..fixing existing code could happen in other issue, unless it is blocking us here, which i doubt?

thanks

wrong patch though? #11 is double size

1. +++ b/core/lib/Drupal/Core/Transliteration/PHPTransliteration.php
   @@ -18,6 +19,29 @@
   +  public function __construct($data_directory = NULL, ModuleHandlerInterface $module_handler) {
   

   it just occured to me that the optional argument should be last, right?

2. +++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
   @@ -74,45 +102,31 @@ public function testPHPTransliteration() {
   +      // array('zz', $two_byte, 'Z O U A O aouaohello'),
   +      // array('zz', $random, $random),
   +      // array('zz', $five_byte, 'ATh', $five_byte_printable),
   

   hmm also what about those? why commented?

The points of paris got adressed.

Er. They did? How?

Oh that is embarrassing.

Needs reroll and fixing #21

Those cases were already moved to the other test class, so let's just remove them here :)

Thanks, looks good..just a small nit, in case this needs to be rerolled

+++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
@@ -31,9 +30,40 @@ public static function getInfo() {
+  public static function providerTestPhpTransliteration() {

does not need to be static

Conflicted in the removed info file, removed the static, good point.

thanks!

Drupal\system\Tests\Cache\ApcuBackendUnitTest random failure

30: transliteration-phpunit-2051467-30.patch queued for re-testing.

30: transliteration-phpunit-2051467-30.patch queued for re-testing.

30: transliteration-phpunit-2051467-30.patch queued for re-testing.

30: transliteration-phpunit-2051467-30.patch queued for re-testing.

finally

This sadly had a merge conflict when I tried to reroll it for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 so it will need a closer look.

Removing the 'phpunit' tag — let's stop to tag every issue that just happens to add or touch phpunit tests, because that makes no sense; we're not adding a tag to all issues that happen to add or touch simpletest tests either. ;)

1. +++ b/core/lib/Drupal/Core/Transliteration/PHPTransliteration.php
   @@ -18,6 +19,29 @@
   +  public function __construct($data_directory = NULL, ModuleHandlerInterface $module_handler) {
   +    parent::__construct($data_directory);
   

   That's an edge-case of required/optional argument ordering, but due to the parent class constructor, I agree that the order is sensible here.

2. +++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
   @@ -31,9 +30,40 @@ public static function getInfo() {
   +  public function testPhpTransliteration($langcode, $original, $expected, $unknown_character = '?', $max_length = NULL) {
   

   Normally, $expected should be taken first (especially in this case), but upon looking further, I just realized that you're trying to keep this conversion patch as small as possible.

3. +++ b/core/tests/Drupal/Tests/Core/Transliteration/PhpTransliterationTest.php
   @@ -0,0 +1,105 @@
   + * @group Drupal
   

   s/@group Drupal/@group Transliteration/g

Removing the 'phpunit' tag — let's stop to tag every issue that just happens to add or touch phpunit tests, because that makes no sense; we're not adding a tag to all issues that happen to add or touch simpletest tests either. ;)

It used to be a perfect tool to push the usage of phpunit by getting more reviews on those issues.

1. +++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
   @@ -2,24 +2,23 @@
   + * @see \Drupal\Component\Transliteration\PhpTransliteration
   
   +++ b/core/tests/Drupal/Tests/Core/Transliteration/PhpTransliterationTest.php
   @@ -0,0 +1,105 @@
   + * @see \Drupal\Core\Transliteration\PhpTransliteration
   + */
   +class PhpTransliterationTest extends UnitTestCase {
   

   Let's use @coversClass

2. +++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
   @@ -2,24 +2,23 @@
      public static function getInfo() {
        return array(
   
   +++ b/core/tests/Drupal/Tests/Core/Transliteration/PhpTransliterationTest.php
   @@ -0,0 +1,105 @@
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Transliteration functionality',
   +      'description' => 'Tests Transliteration component functionality.',
   +      'group' => 'Transliteration',
   +    );
   +  }
   +
   

   We can drop those now.

3. +++ b/core/tests/Drupal/Tests/Component/Transliteration/PhpTransliterationTest.php
   @@ -31,9 +30,40 @@ public static function getInfo() {
   +    $random = $random_generator->String(10);
   

   it looks so wrong to use Uppercase methods.

reroll, and fix #42.3 and #43.

bad merge.

Patch doesn't apply.

patch seems to apply. re running tests.

Rerolled and reviewed locally, looks good to go if the testbot agrees.

Looks like an easy fix.

Needed some re-roll love.

When this comes back green, I'll mark this as postponed due to the class rename issue above #2388169: Rename PHPTransliteration to PhpTransliteration. It will just need a re-roll and not much else.

Rerolled the patch, still looks good.

This changes 99% test code and inject the module handler in PhpTransliteration so is not disruptive. Committed c8c1ae1 and pushed to 8.0.x. Thanks!