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

          Jean-Philippe Gaudreau created issue -
          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.
          Michael de Raadt made changes -
          Field Original Value New Value
          Fix Version/s STABLE backlog [ 10463 ]
          Labels triaged
          Michael de Raadt made changes -
          Link This issue has a non-specific relationship to MDL-28140 [ MDL-28140 ]
          Michael de Raadt made changes -
          Link This issue has a non-specific relationship to MDL-27983 [ MDL-27983 ]
          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.
          Eloy Lafuente (stronk7) made changes -
          Link This issue is duplicated by MDL-27489 [ MDL-27489 ]
          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!
          Jean-Philippe Gaudreau made changes -
          Description Many errors when running unit tests with other languages than English (EN) with Moodle 2.1.2

          Info on a patch already applied in our repository:

          Pull from Repository: git@github.com:StudiumDevel/moodle.git
          Pull MOODLE_21_STABLE Branch: unit_tests_errors_lang
          Pull MOODLE_21_STABLE Diff URL: [https://github.com/StudiumDevel/moodle/compare/MOODLE_21_STABLE...unit_tests_errors_lang]
          Many errors when running unit tests with other languages than English (EN) with Moodle 2.1.2
          Pull 2.1 Branch unit_tests_errors_lang
          Pull 2.1 Diff URL https://github.com/StudiumDevel/moodle/compare/MOODLE_21_STABLE...unit_tests_errors_lang
          Pull from Repository git@github.com:StudiumDevel/moodle.git
          Jean-Philippe Gaudreau made changes -
          Labels triaged patch triaged
          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)?
          Jean-Philippe Gaudreau made changes -
          Pull 2.1 Branch unit_tests_errors_lang MDL-30307_unit_tests_lang_errors
          Pull 2.1 Diff URL https://github.com/StudiumDevel/moodle/compare/MOODLE_21_STABLE...unit_tests_errors_lang https://github.com/StudiUM/moodle/compare/MOODLE_21_STABLE...MDL-30307_unit_tests_lang_errors
          Affects Version/s 2.1 [ 10370 ]
          Affects Version/s 2.1.2 [ 10851 ]
          Jean-Philippe Gaudreau made changes -
          Pull 2.2 Diff URL https://github.com/StudiUM/moodle/compare/MOODLE_22_STABLE...MDL-30307_unit_tests_lang_errors_moodle22
          Pull 2.2 Branch MDL-30307_unit_tests_lang_errors_moodle22
          Affects Version/s 2.2 [ 10656 ]
          Jean-Philippe Gaudreau made changes -
          Pull from Repository git@github.com:StudiumDevel/moodle.git git@github.com:StudiUM/moodle.git
          Jean-Philippe Gaudreau made changes -
          Pull Master Diff URL https://github.com/StudiUM/moodle/compare/master...MDL-30307_unit_tests_lang_errors
          Pull Master Branch MDL-30307_unit_tests_lang_errors
          Priority Minor [ 4 ] Major [ 3 ]
          Affects Version/s 2.3 [ 10657 ]
          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.
          Jean-Philippe Gaudreau made changes -
          Description Many errors when running unit tests with other languages than English (EN) with Moodle 2.1.2
          Many errors when running unit tests with other languages than English (EN)
          Jean-Philippe Gaudreau made changes -
          Link This issue testing discovered MDL-31516 [ MDL-31516 ]
          Michael de Raadt made changes -
          Link This issue will be resolved by MDL-31516 [ MDL-31516 ]
          Michael de Raadt made changes -
          Link This issue testing discovered MDL-31516 [ MDL-31516 ]
          Frédéric Massart made changes -
          Assignee Eloy Lafuente (stronk7) [ stronk7 ] Frédéric Massart [ fred ]
          Frédéric Massart made changes -
          Fix Version/s STABLE Sprint 23 Alpha [ 12358 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Frédéric Massart made changes -
          Fix Version/s STABLE Sprint 23 Omega [ 12362 ]
          Fix Version/s STABLE Sprint 23 Alpha [ 12358 ]
          Frédéric Massart made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          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
          Frédéric Massart made changes -
          Testing Instructions 1/ As an administrator, set your profile language to "Français (FR)"
          2/ Go to "Administration du site > Développement > Tests" and click on "Lancer les tests"

          You should see a lot of errors because of there is no translation to "Français (FR)" in the test
          *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.
          Affects Version/s 2.3 [ 10657 ]
          Frédéric Massart made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Rajesh Taneja made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer rajeshtaneja
          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.
          Rajesh Taneja made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          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())
          Frédéric Massart made changes -
          Status Development in progress [ 3 ] Closed [ 6 ]
          Resolution Won't Fix [ 2 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s STABLE Sprint 23 Omega [ 12362 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved: