Moodle
  1. Moodle
  2. MDL-37476

Unit test fails if course completion is enabled

    Details

    • Rank:
      47115

      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.

        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: