Moodle
  1. Moodle
  2. MDL-35190

random failures in course externallib test

    Details

    • Rank:
      43827

      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.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment - - edited

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

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

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

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

          This looks good Jerome.

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

          Thanks Rosie, submitting to integration.

          Show
          Jérôme Mouneyrac added a comment - Thanks Rosie, submitting to integration.
          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

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

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

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

          Thanks Eloy, fixing it.

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

          sent back to integration

          Show
          Jérôme Mouneyrac added a comment - sent back 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 Jerome, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Jerome, this has been integrated now
          Hide
          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
          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
          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
          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: