Moodle
  1. Moodle
  2. MDL-30307

Errors in unit tests when language is not English (EN)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.1, 2.2
    • Fix Version/s: None
    • Component/s: Unit tests
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Test pre-requisites

      • Install the french language pack (Français FR)

      Test steps

      1. As an administrator, set both your preferred language (in your profile settings), and current language to "Français (FR)"
      2. Go to "Administration du site > Développement > Tests" and click on "Lancer les tests"
      3. Make sure the tests end up all fine.
      4. Edit your profile and set your preferred language to English, as well as your current language.
      5. Make sure the tests end up all fine.
      Show
      Test pre-requisites Install the french language pack (Français FR) Test steps As an administrator, set both your preferred language (in your profile settings), and current language to "Français (FR)" Go to "Administration du site > Développement > Tests" and click on "Lancer les tests" Make sure the tests end up all fine. Edit your profile and set your preferred language to English, as well as your current language. Make sure the tests end up all fine.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Rank:
      32655

      Description

      Many errors when running unit tests with other languages than English (EN)

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I'm not sure if these errors are caused by the way the unit tests are written or because of the collation used in your DB. Eloy might be able to comment on this.

          Your suggested improvements are worth looking at.

          Show
          Michael de Raadt added a comment - I'm not sure if these errors are caused by the way the unit tests are written or because of the collation used in your DB. Eloy might be able to comment on this. Your suggested improvements are worth looking at.
          Hide
          Jean-Philippe Gaudreau added a comment - - edited

          Nevermind the first 3 errors about collations. My fix is for the others about the "." (EN) versus "," (FR) and other strings in unit tests that should be taken from the language packages instead of hardcoded in english.

          Show
          Jean-Philippe Gaudreau added a comment - - edited Nevermind the first 3 errors about collations. My fix is for the others about the "." (EN) versus "," (FR) and other strings in unit tests that should be taken from the language packages instead of hardcoded in english.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've requested comments from Tim Hunt (@ HQ chat) about this patch and here there are them:

          <timhunt> I don't like using grade_edit_tree::format_number for the expected value. It makes the test largely pointless.
          <timhunt> I would test against str_repace('.', get_string('decsep', 'langconfig'), $numoutput[$i])
          <timhunt> Which is ugly, but makes the test stricter.
          <timhunt> But the changes elsewhere where you have done format_float(2.0000000, 7) are good, I think.

          So I think it would be worth changing the way grade_edit_tree ones are doing, then run them under a few languages and apply this both to 2.1 and master (2.2 in weeks).

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've requested comments from Tim Hunt (@ HQ chat) about this patch and here there are them: <timhunt> I don't like using grade_edit_tree::format_number for the expected value. It makes the test largely pointless. <timhunt> I would test against str_repace('.', get_string('decsep', 'langconfig'), $numoutput [$i] ) <timhunt> Which is ugly, but makes the test stricter. <timhunt> But the changes elsewhere where you have done format_float(2.0000000, 7) are good, I think. So I think it would be worth changing the way grade_edit_tree ones are doing, then run them under a few languages and apply this both to 2.1 and master (2.2 in weeks). Thanks and ciao
          Hide
          Jean-Philippe Gaudreau added a comment -

          I think the fix in grade/simpletest/testedittree.php must also be done in MOODLE_20_STABLE.

          Thanks for the feedback!

          Show
          Jean-Philippe Gaudreau added a comment - I think the fix in grade/simpletest/testedittree.php must also be done in MOODLE_20_STABLE. Thanks for the feedback!
          Hide
          Jean-Philippe Gaudreau added a comment -

          sorry for the delay...

          I've fix the unit tests on grade_edit_tree::format_number. It works fine but shows there's a bug in the grade_edit_tree::format_number method :S. Same thing as in the unit tests, the decimal point is hardcoded as a '.', so in French (other language) the function returns '1,' instead of '1'.

              function format_number($number) {
                  $formatted = rtrim(format_float($number, 4),'0');
                  if (substr($formatted, -1)=='.') { //if last char is the decimal point
                      $formatted .= '0';
                  }
                  return $formatted;
              }
          

          Should be :

              function format_number($number) {
                  $formatted = rtrim(format_float($number, 4),'0');
                  if (substr($formatted, -1)==get_string('decsep', 'langconfig')) { //if last char is the decimal point
                      $formatted .= '0';
                  }
                  return $formatted;
              }
          

          2 Questions for you :

          • Should I create another task to fix the bug? If so, how can I test this function in Moodle (testing intructions)?
          • Do you still want me to apply the fixes of this task in 2.2 and 2.3dev (master)?
          Show
          Jean-Philippe Gaudreau added a comment - sorry for the delay... I've fix the unit tests on grade_edit_tree::format_number. It works fine but shows there's a bug in the grade_edit_tree::format_number method :S. Same thing as in the unit tests, the decimal point is hardcoded as a '.', so in French (other language) the function returns '1,' instead of '1'. function format_number($number) { $formatted = rtrim(format_float($number, 4),'0'); if (substr($formatted, -1)=='.') { // if last char is the decimal point $formatted .= '0'; } return $formatted; } Should be : function format_number($number) { $formatted = rtrim(format_float($number, 4),'0'); if (substr($formatted, -1)==get_string('decsep', 'langconfig')) { // if last char is the decimal point $formatted .= '0'; } return $formatted; } 2 Questions for you : Should I create another task to fix the bug? If so, how can I test this function in Moodle (testing intructions)? Do you still want me to apply the fixes of this task in 2.2 and 2.3dev (master)?
          Hide
          Jean-Philippe Gaudreau added a comment -

          Fixes integrated and tested in 2.2 stable and 2.3dev (master).

          Note : grade_edit_tree::format_number bug is still in 2.3dev. I'll create an issue to fix it.

          Show
          Jean-Philippe Gaudreau added a comment - Fixes integrated and tested in 2.2 stable and 2.3dev (master). Note : grade_edit_tree::format_number bug is still in 2.3dev. I'll create an issue to fix it.
          Hide
          Frédéric Massart added a comment -

          Hi, thanks for the patch Jean-Philippe. I have created a branch on my repository to amend to commit message and fix a few white spaces issues.

          Integrators, I don't think we should port this change to 2.3 or master because we are handling users differently, and IMO those changes should only be implemented when we decide to run the tests on different user configurations.

          Here are the original branches and patches from Jean-Philippe:

          git@github.com:StudiUM/moodle.git
          
          - MDL-30307_unit_tests_lang_errors_moodle21
            https://github.com/StudiUM/moodle/compare/MOODLE_21_STABLE...MDL-30307_unit_tests_lang_errors_moodle21
          
          - MDL-30307_unit_tests_lang_errors_moodle22
            https://github.com/StudiUM/moodle/compare/MOODLE_22_STABLE...MDL-30307_unit_tests_lang_errors_moodle22
          
          - MDL-30307_unit_tests_lang_errors
            https://github.com/StudiUM/moodle/compare/master...MDL-30307_unit_tests_lang_errors
          
          Show
          Frédéric Massart added a comment - Hi, thanks for the patch Jean-Philippe. I have created a branch on my repository to amend to commit message and fix a few white spaces issues. Integrators, I don't think we should port this change to 2.3 or master because we are handling users differently, and IMO those changes should only be implemented when we decide to run the tests on different user configurations. Here are the original branches and patches from Jean-Philippe: git@github.com:StudiUM/moodle.git - MDL-30307_unit_tests_lang_errors_moodle21 https://github.com/StudiUM/moodle/compare/MOODLE_21_STABLE...MDL-30307_unit_tests_lang_errors_moodle21 - MDL-30307_unit_tests_lang_errors_moodle22 https://github.com/StudiUM/moodle/compare/MOODLE_22_STABLE...MDL-30307_unit_tests_lang_errors_moodle22 - MDL-30307_unit_tests_lang_errors https://github.com/StudiUM/moodle/compare/master...MDL-30307_unit_tests_lang_errors
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          Patch looks good to me, although you might want to consider forcing local to english, rather then using get_string. As moodle development is done in english, and unit tests are for developers, I don't see any reason for strings translation.

          Feel free to push it for integration, if you feel otherwise.

          Show
          Rajesh Taneja added a comment - Thanks Fred, Patch looks good to me, although you might want to consider forcing local to english, rather then using get_string. As moodle development is done in english, and unit tests are for developers, I don't see any reason for strings translation. Feel free to push it for integration, if you feel otherwise.
          Hide
          Dan Poltawski added a comment -

          We discussed this in developer chat and just to summarise my thoughts on this...

          • As its completely resolved in 2.3 by forcing the language to english I think we can close this won't fix if there isn't a quick solution (its unfortunate, but we have bigger issues which affect our users and need to prioritise).
          • But perhaps we really need to fix these tests to be non-language dependent, in the future maybe we'll make the phpunit tests non-lnaguage dependent. (Though the more I think about it the less I think of that idea). I think if we do this then we'll need to fix this and other issues and probably best handled as part of that change.
          Show
          Dan Poltawski added a comment - We discussed this in developer chat and just to summarise my thoughts on this... As its completely resolved in 2.3 by forcing the language to english I think we can close this won't fix if there isn't a quick solution (its unfortunate, but we have bigger issues which affect our users and need to prioritise). But perhaps we really need to fix these tests to be non-language dependent, in the future maybe we'll make the phpunit tests non-lnaguage dependent. (Though the more I think about it the less I think of that idea). I think if we do this then we'll need to fix this and other issues and probably best handled as part of that change.
          Hide
          Frédéric Massart added a comment -

          As discussed, I am closing this as a Won't Fix. IMO, for the test we're writing from now, we should rely on English by default and explicitly test other languages if we want to. Which is probably a good idea in some cases (only a few) to check if some rules are applied. (cf. format_float())

          Show
          Frédéric Massart added a comment - As discussed, I am closing this as a Won't Fix. IMO, for the test we're writing from now, we should rely on English by default and explicitly test other languages if we want to. Which is probably a good idea in some cases (only a few) to check if some rules are applied. (cf. format_float())

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: