i18next / i18next-parser

Parse your code to extract translation keys/values and manage your catalog files
MIT License
486 stars 198 forks source link

Breaking test: generates one plural key for unkown languages #1069

Open jmlavoier opened 2 months ago

jmlavoier commented 2 months ago

🐛 Bug Report

Running the yarn test

parser
    options
      1) generates one plural key for unknow languages

  0 passing (11ms)
  1 failing

  1) parser
       options
         generates one plural key for unknow languages:

      Uncaught AssertionError: expected { 'test {{count}}_one': '', …(2) } to deeply equal { 'test {{count}}_one': '', …(1) }
      + expected - actual

       {
      -  "test {{count}}_many": ""
         "test {{count}}_one": ""
         "test {{count}}_other": ""
       }

To Reproduce

$ yarn
$ yarn test

Expected behavior

Taking into consideration that I haven't changed anything in the codebase and I'm running it on the master branch, which is supposed to be the production-ready version, the test should run successfully.

 272 passing (18s)
  0 failing

Your Environment

jmlavoier commented 2 months ago

Grooming

Three points to consider to solve this problem.

  1. The problem is on i18next in node environment
  2. The problem is on the test
  3. The problem is that we're doing a wrong test, testing the result of i18next.

1. The problem is on i18next in node environment

I've tested the i18next in node environment and the browser and got different results. I realized that the many possibility is coming only on node environment.

I.e:

This simple implementation got different results:

import i18next from "i18next";

i18next.init({
  pluralSeparator: "_",
  nsSeparator: ":",
});

i18next.services.pluralResolver
  .getSuffixes("unknown", { ordinal: undefined })
  .forEach((suffix) => {
    console.log("suffix", suffix);
  });

Browser (Brave: v1.69.162 - Chromium 128.0.6613.120)

suffix:  _one
suffix:  _other

In this case, the test should not fail.

Node v18.19.1

suffix _one
suffix _many
suffix _other

Due to the many the test is failing

For both tests I used i18next v23.5.1

So if there is a compatibility issue, maybe it's necessary identify what's needs to be fixed to make it consistent on both environments. I know that it should not be executed on browser, but considering that the test is breaking in node environment that is the more common to be executed.

2. The problem is on the test

Considering that the many is not a problem, we would change the test to accept the many.

i18nextParser.once('end', () => {
        assert.deepEqual(result, {
          'test {{count}}_one': '',
          'test {{count}}_many': '', // <- Add this to the assert 
          'test {{count}}_other': '',
        })
        done()
      })

3. The problem is that we're doing a wrong test, testing the result of i18next.

Finally, we should question whether this test is necessary. Since this behavior is part of the i18next library, testing it here introduces unnecessary dependency on external behavior. The responsibility of pluralization lies with i18next, not our library. If there is an issue with i18next, the problem should be addressed within that library, not through our tests.

Conclusion

Let's discuss about these points and decide which solution fits better to this issue so that I can create a PR to sort it out.