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

          Attachments

            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