The current tests in testGetPluralFormat() do test the plural evaluator. But since the code is fairly complex, it can use some additional test. Below is the code I have used to manually test the code during development. It can be used to create additional test. Take a few complex formula's and compare the evaluatePlural() result with the computed result. Of course instead of eval() the test should use hardcoded formula's

<?php
  Drupal::state()->delete('locale.translation.plurals');
  $plurals[0] = 'nplurals=1; plural=0;';
  $eval_plural[0] = '(0)';
  $plurals[1] = 'nplurals=2; plural=(n > 1);';
  $eval_plural[1] = '(n > 1)';
  $plurals[2] = 'nplurals=2; plural=(n!=1);';
  $eval_plural[2] = 'n!=1';
  $plurals[3] = 'nplurals=2; plural=(((n==1)||((n%10)==1))?(0):1);';
  $eval_plural[3] = '(((n==1)||((n%10)==1))?(0):1)';
  $plurals[4] = 'nplurals=3; plural=((((n%10)==1)&&((n%100)!=11))?(0):(((((n%10)>=2)&&((n%10)<=4))&&(((n%100)<10)||((n%100)>=20)))?(1):2));';
  $eval_plural[4] = '((((n%10)==1)&&((n%100)!=11))?(0):(((((n%10)>=2)&&((n%10)<=4))&&(((n%100)<10)||((n%100)>=20)))?(1):2))';
  $plurals[5] = 'nplurals=3; plural=((n==1)?(0):(((n>=2)&&(n<=4))?(1):2));';
  $eval_plural[5] = '((n==1)?(0):(((n>=2)&&(n<=4))?(1):2))';
  $plurals[6] = 'nplurals=3; plural=((n==1)?(0):(((n==0)||(((n%100)>0)&&((n%100)<20)))?(1):2));';
  $eval_plural[6] = '((n==1)?(0):(((n==0)||(((n%100)>0)&&((n%100)<20)))?(1):2))';
  $plurals[7] = 'nplurals=3; plural=((n==1)?(0):(((((n%10)>=2)&&((n%10)<=4))&&(((n%100)<10)||((n%100)>=20)))?(1):2));';
  $eval_plural[7] = '((n==1)?(0):(((((n%10)>=2)&&((n%10)<=4))&&(((n%100)<10)||((n%100)>=20)))?(1):2))';
  $plurals[8] = 'nplurals=4; plural=(((n==1)||(n==11))?(0):(((n==2)||(n==12))?(1):(((n>2)&&(n<20))?(2):3)));';
  $eval_plural[8] = '(((n==1)||(n==11))?(0):(((n==2)||(n==12))?(1):(((n>2)&&(n<20))?(2):3)))';
  $plurals[9] = 'nplurals=4; plural=(((n%100)==1)?(0):(((n%100)==2)?(1):((((n%100)==3)||((n%100)==4))?(2):3)));';
  $eval_plural[9] = '(((n%100)==1)?(0):(((n%100)==2)?(1):((((n%100)==3)||((n%100)==4))?(2):3)))';
  $plurals[10] = 'nplurals=5; plural=((n==1)?(0):((n==2)?(1):((n<7)?(2):((n<11)?(3):4))));';
  $eval_plural[10] = '((n==1)?(0):((n==2)?(1):((n<7)?(2):((n<11)?(3):4))))';
  $plurals[11] = 'nplurals=6; plural=((n==1)?(0):((n==0)?(1):((n==2)?(2):((((n%100)>=3)&&((n%100)<=10))?(3):((((n%100)>=11)&&((n%100)<=99))?(4):5)))));';
  $eval_plural[11] = '((n==1)?(0):((n==0)?(1):((n==2)?(2):((((n%100)>=3)&&((n%100)<=10))?(3):((((n%100)>=11)&&((n%100)<=99))?(4):5)))))';

  $fault = FALSE;
  for ($i=0; $i < count($eval_plural); $i++) {
    if (!isset($plurals[$i])) continue;
    $p = new Drupal\Component\Gettext\PoHeader;
    $parsed = $p->parsePluralForms($plurals[$i]);
    list($nplurals, $new_plural) = $parsed;
    for ($n = 0; $n <= 199; $n++) {
      $new_plural_n = isset($new_plural[$n]) ? $new_plural[$n] : $new_plural['default'];
      $formula = strtr($eval_plural[$i], array('n' => $n));
      $old_plural[$n] = @eval('return intval(' . $formula . ');');
      if ((int)$old_plural[$n] != (int)$new_plural_n) {
        debug('Difference found at ' . $n . ': ' . $old_plural[$n] . ' versus ' . $new_plural_n);
        $fault = TRUE;
      }
    }
  }
  if (!$fault) {
    debug ('All is well that ends well!');
  }
CommentFileSizeAuthor
#30 interdiff_18.txt11.55 KBmile23
#30 2031441-30.patch8.67 KBmile23
#18 interdiff.txt1.3 KBpenyaskito
#18 plurals-test-2031441-18.patch5.89 KBpenyaskito
#15 plurals-test-2031441-15.patch5.85 KBduaelfr
#15 interdiff.txt911 bytesduaelfr
#9 plurals-test-2031441.interdiff.8-10.txt1.3 KBpenyaskito
#9 plurals-test-2031441-10.patch5.87 KBpenyaskito
#8 plurals-test-2031441.interdiff.6-8.txt6.15 KBpenyaskito
#8 plurals-test-2031441-8.patch5.76 KBpenyaskito
#6 plurals-test-2031441.interdiff.1-6.txt6.25 KBpenyaskito
#6 plurals-test-2031441-6.patch4.05 KBpenyaskito
#1 plurals-test-2031441-1.patch3.67 KBpenyaskito

Comments

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new plurals-test-2031441-1.patch3.67 KB

Created a unittest (phpUnit) with the code above. Still using eval().
To run tests, change into the core directory and then run ./vendor/bin/phpunit --group Gettext.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +PHPUnit 
+++ b/core/tests/Drupal/Tests/Component/Gettext/PoHeaderTest.phpundefined
@@ -0,0 +1,69 @@
+ * @group Gettext

OOOH We should add this in other tests as well. That is cool!

+++ b/core/tests/Drupal/Tests/Component/Gettext/PoHeaderTest.phpundefined
@@ -0,0 +1,69 @@
+  public function testPluralsFormula() {

It seems to be a perfect candidate for using dataProviders. Have a look at some "@dataProvider" tags in other core tests.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

@dawehner, Yes, I started this patch because I wanted to play with @dataprovider :_D
I'm curious if anywhere else we are using an external datasource like csv file, and if it would make sense here.

Pending tasks are:

  • Using @dataprovider.
  • Removing evals and hardcode expected values.
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Assigned: Unassigned » penyaskito

Assign to me.

dawehner’s picture

dawehner
Primary language German
commented

All our current data providers use just php directly.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
StatusFileSize
new plurals-test-2031441-6.patch4.05 KB
new plurals-test-2031441.interdiff.1-6.txt6.25 KB

Added dataprovider method, this is fun stuff :-)

For avoiding eval and hardcoding expected values, we must return arrays with 199 elements.
How could we make that legible? No idea.

sutharsan’s picture

sutharsan commented

There is no need to get all 199 values. You can either only test the retured values or use the default as in locale_get_plural().

+      // Plural formulas are stored as an array for 0-199. 100 is the highest
+      // modulo used but storing 0-99 is not enough because below 100 we often
+      // find exceptions (1, 2, etc).
+      $index = $count > 199 ? 100 + ($count % 100) : $count;
+      $plural_indexes[$langcode][$count] = isset($plural_formulas[$langcode]['formula'][$index]) ? $plural_formulas[$langcode]['formula'][$index] : $plural_formulas[$langcode]['formula']['default'];

and/or you can call each function with a number of random values.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
StatusFileSize
new plurals-test-2031441-8.patch5.76 KB
new plurals-test-2031441.interdiff.6-8.txt6.15 KB

Chatted about this with chx and dawehner, and I didn't fully understand this, now I think I do.
This patch adds really long lines for the expected array, but I think in this case is acceptable.

This makes 265 assertions instead of 2400.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
StatusFileSize
new plurals-test-2031441-10.patch5.87 KB
new plurals-test-2031441.interdiff.8-10.txt1.3 KB

Improved docblocks per chx suggestion.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Issue tags: +sprint

Tagging.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +language-ui

Add tag.

dawehner’s picture

dawehner
Primary language German
commented

I really like the new approach!

+++ b/core/tests/Drupal/Tests/Component/Gettext/PoHeaderTest.phpundefined
@@ -0,0 +1,107 @@
+   * Tests that for the given plural expression, the plural formula is parsed
+   * correctly.
+   * @param string $plural
...
+   * Gets pairs of plural expressions and expected plural positions keyed by
+   * plural value.

The first description should just be one line, try to truncate it to below 80 chars.

After that the coding style forces you to make an emptyu line.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work
Issue tags: -Need tests

.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Assigned: penyaskito » Unassigned
Issue tags: +Novice

Unassigning and tagging with novice, only #12 is pending and I cannot find the time at the moment.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt911 bytes
new plurals-test-2031441-15.patch5.85 KB
dawehner’s picture

dawehner
Primary language German
commented

This is nearly perfect.

+++ b/core/tests/Drupal/Tests/Component/Gettext/PoHeaderTest.phpundefined
@@ -0,0 +1,108 @@
+   *  Array of expected plural positions keyed by plural value.
+   * @dataProvider getPluralFormulasIterator

Let's put an empty line here between.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new plurals-test-2031441-18.patch5.89 KB
new interdiff.txt1.3 KB

I found some other missing blank lines according to our standards. Also corrected one function comment to a single line.

dawehner’s picture

dawehner
Primary language German
commented

There is wrong indentation in quite some docs (sorry I can't use dreditor at the moment).

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue tags: +Needs tests

I found that #18 applies cleanly, but then has one error and fails all the tests. This is with PHP 5.3.14 and 5.4.4.

$ cd drupal/core
$ ./vendor/bin/phpunit

I get this error:

There was 1 error:

1) Drupal\Tests\Component\Gettext\PoHeaderTest::testPluralsFormula with data set #0 ('nplurals=1; plural=0;', array(0))
Illegal string offset 'default'

/Users/paul/pj2/drupal/core/tests/Drupal/Tests/Component/Gettext/PoHeaderTest.php:46

I'd guess that arises out of line 46:
$result = isset($new_plural[$number]) ? $new_plural[$number] : $new_plural['default'];

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue tags: -Needs tests

Hmm. Tag craziness.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

#18: plurals-test-2031441-18.patch queued for re-testing.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Postponed

This fails because depends on #1273968: remove eval from locale.module which was reverted because of the js issue.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -sprint

Remove from sprint, since that one is not on sprint either. Let's focus on our several outstanding major and critical tasks!

yesct’s picture

yesct commented

tag was stuck. trying again to remove sprint tag.

penyaskito queued 18: plurals-test-2031441-18.patch for re-testing.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Postponed » Needs review

Retesting #18, as the blocker issue was fixed.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Issue tags: +sprint
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Issue tags: -Novice

There are not novice tasks here anymore.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2031441-30.patch8.67 KB
new interdiff_18.txt11.55 KB

Removed getInfo() as per: https://www.drupal.org/node/2301125

A ton of coding standards reformatting.

Renamed dataprovider to providerTestPluralsFormula().

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Yay thanks! It is always sad to see patches in limbo for code style but then again we like looking at code we can discern :D

alexpott’s picture

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

Committed e665794 and pushed to 8.x. Thanks!

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

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