Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35190

random failures in course externallib test

    Details

      Description

      Course externallib tested introduced by MDL-33995 fail randomly because they expect get_records() and friends without ORDER BY return consistent results, this caused test_get_courses() to fail randomly.

              $courses = core_course_external::get_courses(array('ids' =>
                  array($course1->id, $course2->id)));
       
              // Check we retrieve the good total number of categories.
              $this->assertEquals(2, count($courses));
       
              // Check the return values for course 1
              $dbcourse = $DB->get_record('course', array('id' => $course1->id));
              $this->assertEquals($courses[0]['id'], $dbcourse->id);

      ORder of courses in $courses variable is not guaranteed.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment - - edited

            It would be probably better to remove deprecated "key_exists" from all externalib files too.

            Show
            skodak Petr Skoda added a comment - - edited It would be probably better to remove deprecated "key_exists" from all externalib files too.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Petr.I created MDL-35198 for the deprecated key_exists usage.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Petr.I created MDL-35198 for the deprecated key_exists usage.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This looks good Jerome.

            Show
            rwijaya Rossiani Wijaya added a comment - This looks good Jerome.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Rosie, submitting to integration.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Rosie, submitting to integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Jerome,

            I really don't think this is the best solution, as far as it can lead to the tests (assertions) being executed or skipped depending of the nature of the DB or whatever.

            So, IMO, a better alternative would be:

            1) Create one $createdcategories array, where you store, indexed by the id of each created category, the category. Basically, something like:

            $createdcats = array(
                $category1->id => $category1,
                $category2->id => $category2,
                ......);

            2) And later the test would be something like:

            foreach ($categories as $category) {
                $currentid = $category['id'];
                $this->assertEquals($category['idnumber'], $createdcats[$currentid]->idnumber);
                $this->assertEquals($category['name'], $createdcats[$currentid]->name);
                ......
            }

            That way, there is not any "random" execution at all. Each category is tested against its original $createdcats object. Same applies to the courses iteration in the patch.

            So I'm about to reopen this... ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Jerome, I really don't think this is the best solution, as far as it can lead to the tests (assertions) being executed or skipped depending of the nature of the DB or whatever. So, IMO, a better alternative would be: 1) Create one $createdcategories array, where you store, indexed by the id of each created category, the category. Basically, something like: $createdcats = array( $category1->id => $category1, $category2->id => $category2, ......); 2) And later the test would be something like: foreach ($categories as $category) { $currentid = $category['id']; $this->assertEquals($category['idnumber'], $createdcats[$currentid]->idnumber); $this->assertEquals($category['name'], $createdcats[$currentid]->name); ...... } That way, there is not any "random" execution at all. Each category is tested against its original $createdcats object. Same applies to the courses iteration in the patch. So I'm about to reopen this... ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (sorry for the complete cycle, I pressed the incorrect button)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (sorry for the complete cycle, I pressed the incorrect button)
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Eloy, fixing it.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Eloy, fixing it.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            sent back to integration

            Show
            jerome Jérôme Mouneyrac added a comment - sent back to integration
            Hide
            stronk7 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
            stronk7 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks Jerome, this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Jerome, this has been integrated now
            Hide
            timb Tim Barker added a comment -

            Unit tests run on the nightly build machine every night and the Integration CI server at every checkin. There is no need to run PHP unit tests manually on Wednesday.

            Show
            timb Tim Barker added a comment - Unit tests run on the nightly build machine every night and the Integration CI server at every checkin. There is no need to run PHP unit tests manually on Wednesday.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Nov/12