Moodle
  1. Moodle
  2. MDL-37476

Unit test fails if course completion is enabled

    Details

      Description

      Two issues here:

      1. In the test "core_course_external_testcase::test_create_courses" there is a conditional test on L390 that checks completion_info::is_enabled_for_site().

      Because PHPUnit pulls its config from the default for the setting (not from the site's config table) this branch is never executed even if you enable completion on the site you are testing. It will only run if you added "set_config('enablecompletion', 1);" in the test, or if you changed the enablecompletion default to 1 in admin/settings/subsystems.php.

      2. If that branch of code was executed there is a bug, causing a test failure:

      There was 1 error:

      1) core_course_external_testcase::test_create_courses
      Undefined index: enabledcompletion

      /home/simonc/code/mdl22/course/tests/externallib_test.php:392
      /home/simonc/code/mdl22/lib/phpunit/classes/advanced_testcase.php:76

      The issue is that the array key on L391 should be 'enablecompletion' not 'enabledcompletion'.

      The fix for the 2nd problem is to correct the type in the array key. The 1st problem could be fixed by removing the if statement and the 'true' condition completely or by using set_config() and testing with and without completion enabled separately.

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that, Simon.

          Aaron: I've assigned this to you as it is related to Course Completion.

          Show
          Michael de Raadt added a comment - Thanks for reporting that, Simon. Aaron: I've assigned this to you as it is related to Course Completion.
          Hide
          Jérôme Mouneyrac added a comment -

          My bad, I'll fix it. Thanks for the report Simon.

          Show
          Jérôme Mouneyrac added a comment - My bad, I'll fix it. Thanks for the report Simon.
          Hide
          Jérôme Mouneyrac added a comment -

          I kept the "if condition" because I think it's good to let developer know how to write the "if condition", even thought this "if condition" is always true in this test.
          Thanks Simon.

          Show
          Jérôme Mouneyrac added a comment - I kept the "if condition" because I think it's good to let developer know how to write the "if condition", even thought this "if condition" is always true in this test. Thanks Simon.
          Hide
          Rajesh Taneja added a comment -

          Hello Jerome,

          Patch looks good, although it might be nice to remove if statement, as it's not valid anymore.

          Also, looking at the test it seems course3 is not created, hence validation for course 3 is not happening as well.
          Not sure if you would like to fix this as part of this bug.

          Show
          Rajesh Taneja added a comment - Hello Jerome, Patch looks good, although it might be nice to remove if statement, as it's not valid anymore. Also, looking at the test it seems course3 is not created, hence validation for course 3 is not happening as well. Not sure if you would like to fix this as part of this bug.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks, I made the changes, sending to integration.

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks, I made the changes, sending to integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: