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!');
  }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

penyaskito’s picture

Status: Active » Needs review
FileSize
3.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

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

@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

Assigned: Unassigned » penyaskito

Assign to me.

dawehner’s picture

All our current data providers use just php directly.

penyaskito’s picture

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

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

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

Improved docblocks per chx suggestion.

penyaskito’s picture

Issue tags: +sprint

Tagging.

Gábor Hojtsy’s picture

Issue tags: +language-ui

Add tag.

dawehner’s picture

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

Status: Needs review » Needs work
Issue tags: -Need tests

.

penyaskito’s picture

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

Status: Needs work » Needs review
FileSize
911 bytes
5.85 KB
dawehner’s picture

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

Status: Needs review » Needs work
penyaskito’s picture

Status: Needs work » Needs review
FileSize
5.89 KB
1.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

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

Mile23’s picture

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

Issue tags: -Needs tests

Hmm. Tag craziness.

Gábor Hojtsy’s picture

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

penyaskito’s picture

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

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

tag was stuck. trying again to remove sprint tag.

penyaskito’s picture

Status: Postponed » Needs review

Retesting #18, as the blocker issue was fixed.

penyaskito’s picture

Issue tags: +sprint
penyaskito’s picture

Issue tags: -Novice

There are not novice tasks here anymore.

Mile23’s picture

FileSize
8.67 KB
11.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

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

Status: Reviewed & tested by the community » Fixed

Committed e665794 and pushed to 8.x. Thanks!

penyaskito’s picture

Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

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