Moodle
  1. Moodle
  2. MDL-36315

Minimize the time-window where backup_cron_helper_testcase is failing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.4
    • Fix Version/s: 2.3.3
    • Component/s: Backup
    • Labels:
      None
    • Testing Instructions:
      Hide

      Set the server clock to various dates/times and execute:

      phpunit backup_cron_helper_testcase backup/util/helper/tests/cronhelper_test.php
      
      • November 1st, 00:30 => test execution passes (was failing before the patch).
      • November 2nd, 00:30 => test execution passes (was failing before the patch).
      • November 3rd, 00:30 => test execution passes (was failing before the patch).
      • November 4th, 00:30 => test execution FAILS (coz it's "around" when New_York DST change happens).
      • November 5th, 00:30 => test execution passes.

      You also can try with dates around Brussels DST changes (last Sunday of March and October). The test execution will only fail for some hours around 02:00 AM. Previously it was failing for up-to a week.

      Note that the exact dates/times used above have been set in one computer under Europe/Madrid TZ, so results may change if another TZ is used in the testing machine. In any case, the tests should NOT be failing for more than 48h "around" the DST change moment anymore.

      Ciao

      Show
      Set the server clock to various dates/times and execute: phpunit backup_cron_helper_testcase backup/util/helper/tests/cronhelper_test.php November 1st, 00:30 => test execution passes (was failing before the patch). November 2nd, 00:30 => test execution passes (was failing before the patch). November 3rd, 00:30 => test execution passes (was failing before the patch). November 4th, 00:30 => test execution FAILS (coz it's "around" when New_York DST change happens). November 5th, 00:30 => test execution passes. You also can try with dates around Brussels DST changes (last Sunday of March and October). The test execution will only fail for some hours around 02:00 AM. Previously it was failing for up-to a week. Note that the exact dates/times used above have been set in one computer under Europe/Madrid TZ, so results may change if another TZ is used in the testing machine. In any case, the tests should NOT be failing for more than 48h "around" the DST change moment anymore. Ciao
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      45108

      Description

      This is a followup of MDL-28531.

      it has been detected that the tests added to verify the behavior of calculate_next_automated_backup() are failing "too much".

      For example, since some days ago, with the next DST change for America/New_York coming next Sunday, a bunch of tests are failing. Same applies to the Europe/Brussels one 1 week ago.

      The main reason for those longs periods getting failed tests is that the calculation of the $dst variable is performed for time() and that causes next executions scheduled to return results with DST changes applied for long periods (up to 'next Monday 18:00:00' in some cases.

      So one immediate solution to REDUCE (not to fix them forever) is to, simply, change the $dst calculation, from:

      $dst = date('I');
      

      to:

      $dst = date('I', $now);
      

      That way, the failing window is reduced a lot and only when executed "around" the DST changes (some hours before to some hours after) the unit tests will fail.

      The proposed patch applies that change in the 4 places susceptible to get a "reduced" failing window, with explanations about when the tests are supposed to continue failing.

      Ciao

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          sending this to Fred for P2P review, sure he's the man for this.

          Show
          Eloy Lafuente (stronk7) added a comment - sending this to Fred for P2P review, sure he's the man for this.
          Hide
          Frédéric Massart added a comment -

          Thanks for fixing my mistakes Eloy! I should have thought of the DST a little bit more.

          As you said, this will not resolve the entire DST/TZ issue, but it will greatly help for now. I'm pushing this for integration!

          Cheers!

          Show
          Frédéric Massart added a comment - Thanks for fixing my mistakes Eloy! I should have thought of the DST a little bit more. As you said, this will not resolve the entire DST/TZ issue, but it will greatly help for now. I'm pushing this for integration! Cheers!
          Hide
          Dan Poltawski added a comment -

          Thanks Eloy and Fred, this has been integrated now.

          Show
          Dan Poltawski added a comment - Thanks Eloy and Fred, this has been integrated now.
          Hide
          David Monllaó added a comment -

          It passes. Tested in master with UTC +8, adding to the specified scenarios I've tested with:

          • "4 NOV 2012 17:30:00" -> passes
          • "4 NOV 2012 10:30:00" -> fails
          Show
          David Monllaó added a comment - It passes. Tested in master with UTC +8, adding to the specified scenarios I've tested with: "4 NOV 2012 17:30:00" -> passes "4 NOV 2012 10:30:00" -> fails
          Hide
          Dan Poltawski added a comment -

          Hurray!

          You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

          Show
          Dan Poltawski added a comment - Hurray! You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

            People

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

              Dates

              • Created:
                Updated:
                Resolved: