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

Cron fails to remove directories from temp that are older than 1 week

    Details

    • Testing Instructions:
      Hide

      1. Create the following in moodledata/temp:
      mkdir -p dir_2/dir_2_1
      touch -t 04070101 dir_2/dir_2_1/file_2_1_1
      touch -t 04070101 dir_2/dir_2_1
      touch -t 04070101 dir_2

      2. Runs in cron. You will need to run cron multiple times until the entry ' Deleting temporary files...' appears, you should then get the error 'Failed removing directory '

      {moodledata}

      /temp/dir_2'.

      Once this fix has been applied, this error should not be displayed.

      Show
      1. Create the following in moodledata/temp: mkdir -p dir_2/dir_2_1 touch -t 04070101 dir_2/dir_2_1/file_2_1_1 touch -t 04070101 dir_2/dir_2_1 touch -t 04070101 dir_2 2. Runs in cron. You will need to run cron multiple times until the entry ' Deleting temporary files...' appears, you should then get the error 'Failed removing directory ' {moodledata} /temp/dir_2'. Once this fix has been applied, this error should not be displayed.
    • Affected Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
    • Pull Master Branch:
      MDL-45654-master

      Description

      When the cron function cron_delete_from_temp() runs and attempts
      to remove anything older that 1 week, the directory remove will fail for a directory where a file has already been deleted from a child directory of this directory. This is since the mtime of the child directory is updated to whenever the file is deleted, which is not older that 1 week. But the parent directory is still in the list to be deleted. So you get this error that the deletion of the directory failed:

      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/654000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/546000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/480000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/600000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/675000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/119000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/47000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/754000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/755000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/417000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/720000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/788000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/455000'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.56.1236804135/user_files/461000'.
      ......
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.754.325959266/moddata/oucontent'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.6.283880280/course_files/hires/a330_VS_Plate_2_4_DONE'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.6.283880280/course_files/hires'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.6.283880280/course_files'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.6.871256423/course_files/hires/a330_VS_Plate_2_4_DONE'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.6.871256423/course_files/hires'.
      Failed removing directory '/vle/learn3acct/moodledata/temp/local_convertoldbackup.6.871256423/course_files'.
      

      if you create the following in moodledata/temp:

      mkdir -p dir_2/dir_2_1
      touch -t 04070101 dir_2/dir_2_1/file_2_1_1
      touch -t 04070101 dir_2/dir_2_1
      touch -t 04070101 dir_2
      

      when cron_delete_from_temp() runs in cron ( only every 1 in 5 ish), it should fail to delete dir_2 since dir_2_1 still exists but file_2_1_1 was deleted.

      I found that since this function uses the Directory Iterator etc, the simplest fix would be to check if the Directory has any Children, unfortunately this doesn't seem to work correctly, I think due to the '.' and '..' entries.

      The solution I have found that has minimal code change is to use the glob function which ignores the '.' and '..' entries.

      In investigating this issue, I found that the unit test, /lib/tests/cronlib_test.php, has a unit test for this functionality but the timestamps used do not seem to be what was intended.

      Checked phpunit tests on linux (centos6) and windows (windows7) with php 5.5 and Postgresql

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            rtcn2 Rod Norfor added a comment -

            This functionality was originally added to 2.6 with this tracker issue https://tracker.moodle.org/browse/MDL-38570

            Show
            rtcn2 Rod Norfor added a comment - This functionality was originally added to 2.6 with this tracker issue https://tracker.moodle.org/browse/MDL-38570
            Hide
            salvetore Michael de Raadt added a comment -

            I think this problem could lead to big problems if it occurs, so I've bumped the priority on this issue.

            Thanks for reporting it and providing a patches.

            Show
            salvetore Michael de Raadt added a comment - I think this problem could lead to big problems if it occurs, so I've bumped the priority on this issue. Thanks for reporting it and providing a patches.
            Hide
            quen Sam Marshall added a comment -

            +1 from me (I looked at this earlier while Rod was developing it)

            Note: I don't think this is critical as the only problem it causes is that you get a lot of unnecessary warnings in the cron log. This makes it harder to detect real problems, so it's serious, but does not have a practical consequence otherwise. The folders do still get deleted - eventually - it just causes a bunch of warnings in the process.

            The code change here is quite simple, it's basically about not attempting to delete a directory if it is not empty.

            The reason that it would attempt to do this is more complicated (child directories are not deleted, because the directory modified time is updated during the process of deleting other files, so you have an 'old' directory but it contains 'new' directories). Rod actually considered a number of other approaches to improve it, but we decided this was the simplest way to change the code and also probably the best..

            There is a more significant change to the unit test because there were lots of bugs in it before, it basically only passed by coincidence.

            Show
            quen Sam Marshall added a comment - +1 from me (I looked at this earlier while Rod was developing it) Note: I don't think this is critical as the only problem it causes is that you get a lot of unnecessary warnings in the cron log. This makes it harder to detect real problems, so it's serious, but does not have a practical consequence otherwise. The folders do still get deleted - eventually - it just causes a bunch of warnings in the process. The code change here is quite simple, it's basically about not attempting to delete a directory if it is not empty. The reason that it would attempt to do this is more complicated (child directories are not deleted, because the directory modified time is updated during the process of deleting other files, so you have an 'old' directory but it contains 'new' directories). Rod actually considered a number of other approaches to improve it, but we decided this was the simplest way to change the code and also probably the best.. There is a more significant change to the unit test because there were lots of bugs in it before, it basically only passed by coincidence.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Rod - looks good to me and has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Rod - looks good to me and has been integrated now.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Works as described, I didn't see the error.

            Show
            ankit_frenz Ankit Agarwal added a comment - Works as described, I didn't see the error.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Sorry guys,

            I am failing this for now, to ensure this is not causing the failure which Michael de Raadt is observing on 26 integration

            1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass
            , stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass
            , stdClass, stdClass, stdClass, stdClass), array('D:\\xampp\\data\\26_integratio
            n_PostgreSQL_phpunit\\temp\\dir1', 'D:\\xampp\\data\\26_integration_PostgreSQL_p
            hpunit\\temp\\dir1\\dir1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit
            \\temp\\dir1\\dir1_1\\dir1_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_php
            unit\\temp\\dir1\\dir1_1\\dir1_1_1\\file1_1_1_2', 'D:\\xampp\\data\\26_integrati
            on_PostgreSQL_phpunit\\temp\\dir1\\dir1_2', 'D:\\xampp\\data\\26_integration_Pos
            tgreSQL_phpunit\\temp\\dir1\\dir1_2\\file1_1_2_2', 'D:\\xampp\\data\\26_integrat
            ion_PostgreSQL_phpunit\\temp\\dir2', 'D:\\xampp\\data\\26_integration_PostgreSQL
            _phpunit\\temp\\dir4', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp
            \\dir4\\dir4_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\dir4
            \\dir4_1\\dir4_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\
            file1'))
            Failed asserting that two arrays are equal.
            --- Expected
            +++ Actual
            @@ @@
                 5 => 'D:\xampp\data\26_integration_..._1_2_2'
            -    6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir2'
            -    7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4'
            -    8 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1'
            -    9 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1\dir4
            _1_1'
            -    10 => 'D:\xampp\data\26_integration_...\file1'
            +    6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4'
            +    7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1'
            +    8 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1\dir4
            _1_1'
            +    9 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\file1'
             )
             
            D:\xampp\htdocs\26_integration\lib\tests\cronlib_test.php:179
            D:\xampp\htdocs\26_integration\lib\phpunit\classes\basic_testcase.php:64
             
            To re-run:
             D:\xampp\htdocs\26_integration\vendor\bin\/../phpunit/phpunit/composer/bin/phpu
            nit cronlib_testcase lib\tests\cronlib_test.php
             
            FAILURES!
            Tests: 2429, Assertions: 45145, Failures: 1, Skipped: 7.
            

            Show
            rajeshtaneja Rajesh Taneja added a comment - Sorry guys, I am failing this for now, to ensure this is not causing the failure which Michael de Raadt is observing on 26 integration 1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass , stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass , stdClass, stdClass, stdClass, stdClass), array('D:\\xampp\\data\\26_integratio n_PostgreSQL_phpunit\\temp\\dir1', 'D:\\xampp\\data\\26_integration_PostgreSQL_p hpunit\\temp\\dir1\\dir1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit \\temp\\dir1\\dir1_1\\dir1_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_php unit\\temp\\dir1\\dir1_1\\dir1_1_1\\file1_1_1_2', 'D:\\xampp\\data\\26_integrati on_PostgreSQL_phpunit\\temp\\dir1\\dir1_2', 'D:\\xampp\\data\\26_integration_Pos tgreSQL_phpunit\\temp\\dir1\\dir1_2\\file1_1_2_2', 'D:\\xampp\\data\\26_integrat ion_PostgreSQL_phpunit\\temp\\dir2', 'D:\\xampp\\data\\26_integration_PostgreSQL _phpunit\\temp\\dir4', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp \\dir4\\dir4_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\dir4 \\dir4_1\\dir4_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\ file1')) Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ 5 => 'D:\xampp\data\26_integration_..._1_2_2' - 6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir2' - 7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4' - 8 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1' - 9 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1\dir4 _1_1' - 10 => 'D:\xampp\data\26_integration_...\file1' + 6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4' + 7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1' + 8 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1\dir4 _1_1' + 9 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\file1' )   D:\xampp\htdocs\26_integration\lib\tests\cronlib_test.php:179 D:\xampp\htdocs\26_integration\lib\phpunit\classes\basic_testcase.php:64   To re-run: D:\xampp\htdocs\26_integration\vendor\bin\/../phpunit/phpunit/composer/bin/phpu nit cronlib_testcase lib\tests\cronlib_test.php   FAILURES! Tests: 2429, Assertions: 45145, Failures: 1, Skipped: 7.
            Hide
            salvetore Michael de Raadt added a comment -

            After reverting that change, tests pass.

            Perhaps this has to do with the tests running under Windows in my case.

            Show
            salvetore Michael de Raadt added a comment - After reverting that change, tests pass. Perhaps this has to do with the tests running under Windows in my case.
            Hide
            rtcn2 Rod Norfor added a comment -

            I think the test is failing due to the time it is taking the run. The directory it is removing would be exactly 1 week old, but if the time taken to run the test is long enough, it would remove this directory since it would then be over 1 week old.
            This was one of the original tests which looked like it had a timestamp of 1 week old ( a bit confusing since $time - strtotime('1 week') is the same as $time since the format of the function call strtotime is incorrect):
            $weekstime = $time - strtotime('1 week');
            $beforeweekstime = $time - strtotime('1 week') - 1;
            $afterweekstime = $time + strtotime('1 week') + 1;
            ......
            .....
            $node3 = new stdClass();
            $node3->path = '/dir2/';
            $node3->isdir = true;
            $node3->time = $time - $weekstime;

            Should I change the timestamp of the directory 'dir2' to be definitely less than a week old, however long it takes the unit test to run? or something else?

            Show
            rtcn2 Rod Norfor added a comment - I think the test is failing due to the time it is taking the run. The directory it is removing would be exactly 1 week old, but if the time taken to run the test is long enough, it would remove this directory since it would then be over 1 week old. This was one of the original tests which looked like it had a timestamp of 1 week old ( a bit confusing since $time - strtotime('1 week') is the same as $time since the format of the function call strtotime is incorrect): $weekstime = $time - strtotime('1 week'); $beforeweekstime = $time - strtotime('1 week') - 1; $afterweekstime = $time + strtotime('1 week') + 1; ...... ..... $node3 = new stdClass(); $node3->path = '/dir2/'; $node3->isdir = true; $node3->time = $time - $weekstime; Should I change the timestamp of the directory 'dir2' to be definitely less than a week old, however long it takes the unit test to run? or something else?
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Gah.

            It appears data providers get executed and responses collected during the initialisation/setup of PHPUnit as a whole.
            The reason this is failing is because timestamps in the data provider are initialised well before the tests are actually run.
            So it is presently determined by the space of time between PHPUnit starting and it finally getting to the tests to execute.
            This model is bound to fail. We could add an arbitrary time however it would not be guaranteed to always pass.

            I purpose that we convert the absolute timestamps into relative time differences that get applied when touching.

            I'll test that as a solution now.

            Show
            samhemelryk Sam Hemelryk added a comment - Gah. It appears data providers get executed and responses collected during the initialisation/setup of PHPUnit as a whole. The reason this is failing is because timestamps in the data provider are initialised well before the tests are actually run. So it is presently determined by the space of time between PHPUnit starting and it finally getting to the tests to execute. This model is bound to fail. We could add an arbitrary time however it would not be guaranteed to always pass. I purpose that we convert the absolute timestamps into relative time differences that get applied when touching. I'll test that as a solution now.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Ok tested on all branches and now passing full runs.
            Diffs at: http://mdl.icodedthat.com/45654-26~1
            The fix has being integrated now - just needing CI to complete a full test on this for us.

            Show
            samhemelryk Sam Hemelryk added a comment - Ok tested on all branches and now passing full runs. Diffs at: http://mdl.icodedthat.com/45654-26~1 The fix has being integrated now - just needing CI to complete a full test on this for us.
            Hide
            salvetore Michael de Raadt added a comment -

            A very similar error is still occurring for me...

            Time: 12.62 minutes, Memory: 242.50Mb
             
            There was 1 failure:
             
            1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass
            , stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass
            , stdClass, stdClass, stdClass, stdClass), array('D:\\xampp\\data\\26_integratio
            n_PostgreSQL_phpunit\\temp\\dir1', 'D:\\xampp\\data\\26_integration_PostgreSQL_p
            hpunit\\temp\\dir1\\dir1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit
            \\temp\\dir1\\dir1_1\\dir1_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_php
            unit\\temp\\dir1\\dir1_1\\dir1_1_1\\file1_1_1_2', 'D:\\xampp\\data\\26_integrati
            on_PostgreSQL_phpunit\\temp\\dir1\\dir1_2', 'D:\\xampp\\data\\26_integration_Pos
            tgreSQL_phpunit\\temp\\dir1\\dir1_2\\file1_1_2_2', 'D:\\xampp\\data\\26_integrat
            ion_PostgreSQL_phpunit\\temp\\dir2', 'D:\\xampp\\data\\26_integration_PostgreSQL
            _phpunit\\temp\\dir4', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp
            \\dir4\\dir4_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\dir4
            \\dir4_1\\dir4_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\
            file1'))
            Failed asserting that two arrays are equal.
            --- Expected
            +++ Actual
            @@ @@
             Array (
                 0 => 'D:\xampp\data\26_integration_...p\dir1'
                 1 => 'D:\xampp\data\26_integration_...dir1_1'
                 2 => 'D:\xampp\data\26_integration_...r1_1_1'
            -    3 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_1\dir1
            _1_1\file1_1_1_2'
            -    4 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_2'
            -    5 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_2\file
            1_1_2_2'
            -    6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir2'
            -    7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4'
            -    8 => 'D:\xampp\data\26_integration_...dir4_1'
            -    9 => 'D:\xampp\data\26_integration_...r4_1_1'
            -    10 => 'D:\xampp\data\26_integration_...\file1'
            +    3 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_2'
            +    4 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir2'
            +    5 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4'
            +    6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1'
            +    7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1\dir4
            _1_1'
             )
             
            D:\xampp\htdocs\26_integration\lib\tests\cronlib_test.php:177
            D:\xampp\htdocs\26_integration\lib\phpunit\classes\basic_testcase.php:64
             
            To re-run:
             D:\xampp\htdocs\26_integration\vendor\bin\/../phpunit/phpunit/composer/bin/phpu
            nit cronlib_testcase lib\tests\cronlib_test.php
             
            FAILURES!
            Tests: 2440, Assertions: 45215, Failures: 1, Skipped: 7.
            

            Show
            salvetore Michael de Raadt added a comment - A very similar error is still occurring for me... Time: 12.62 minutes, Memory: 242.50Mb   There was 1 failure:   1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass , stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass , stdClass, stdClass, stdClass, stdClass), array('D:\\xampp\\data\\26_integratio n_PostgreSQL_phpunit\\temp\\dir1', 'D:\\xampp\\data\\26_integration_PostgreSQL_p hpunit\\temp\\dir1\\dir1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit \\temp\\dir1\\dir1_1\\dir1_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_php unit\\temp\\dir1\\dir1_1\\dir1_1_1\\file1_1_1_2', 'D:\\xampp\\data\\26_integrati on_PostgreSQL_phpunit\\temp\\dir1\\dir1_2', 'D:\\xampp\\data\\26_integration_Pos tgreSQL_phpunit\\temp\\dir1\\dir1_2\\file1_1_2_2', 'D:\\xampp\\data\\26_integrat ion_PostgreSQL_phpunit\\temp\\dir2', 'D:\\xampp\\data\\26_integration_PostgreSQL _phpunit\\temp\\dir4', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp \\dir4\\dir4_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\dir4 \\dir4_1\\dir4_1_1', 'D:\\xampp\\data\\26_integration_PostgreSQL_phpunit\\temp\\ file1')) Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ Array ( 0 => 'D:\xampp\data\26_integration_...p\dir1' 1 => 'D:\xampp\data\26_integration_...dir1_1' 2 => 'D:\xampp\data\26_integration_...r1_1_1' - 3 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_1\dir1 _1_1\file1_1_1_2' - 4 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_2' - 5 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_2\file 1_1_2_2' - 6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir2' - 7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4' - 8 => 'D:\xampp\data\26_integration_...dir4_1' - 9 => 'D:\xampp\data\26_integration_...r4_1_1' - 10 => 'D:\xampp\data\26_integration_...\file1' + 3 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir1\dir1_2' + 4 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir2' + 5 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4' + 6 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1' + 7 => 'D:\xampp\data\26_integration_PostgreSQL_phpunit\temp\dir4\dir4_1\dir4 _1_1' )   D:\xampp\htdocs\26_integration\lib\tests\cronlib_test.php:177 D:\xampp\htdocs\26_integration\lib\phpunit\classes\basic_testcase.php:64   To re-run: D:\xampp\htdocs\26_integration\vendor\bin\/../phpunit/phpunit/composer/bin/phpu nit cronlib_testcase lib\tests\cronlib_test.php   FAILURES! Tests: 2440, Assertions: 45215, Failures: 1, Skipped: 7.
            Hide
            salvetore Michael de Raadt added a comment -

            I thought it should also be noted that the error occurs when the test is run in isolation as well as during a full run.

            Show
            salvetore Michael de Raadt added a comment - I thought it should also be noted that the error occurs when the test is run in isolation as well as during a full run.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Aha thanks for that Michael.
            I'm looking into that now.
            It's passed locally for me under linux + osx, the public CI server has passed it. But yourself under windows, Eloy's CI server under osx I believe and Raj under windows have failed.
            That gives me a couple of things to suspect as to what is causing these fails.

            Show
            samhemelryk Sam Hemelryk added a comment - Aha thanks for that Michael. I'm looking into that now. It's passed locally for me under linux + osx, the public CI server has passed it. But yourself under windows, Eloy's CI server under osx I believe and Raj under windows have failed. That gives me a couple of things to suspect as to what is causing these fails.
            Hide
            damyon Damyon Wiese added a comment -

            A fix for the windows fail is here:

            master git pull https://github.com/damyon/moodle.git MDL-45654-master
            27 git pull https://github.com/damyon/moodle.git MDL-45654-master
            26 git pull https://github.com/damyon/moodle.git MDL-45654-26

            Show
            damyon Damyon Wiese added a comment - A fix for the windows fail is here: master git pull https://github.com/damyon/moodle.git MDL-45654 -master 27 git pull https://github.com/damyon/moodle.git MDL-45654 -master 26 git pull https://github.com/damyon/moodle.git MDL-45654 -26
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks for finding the fix there Damyon - I've pulled it in now - back to the laptop CI now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for finding the fix there Damyon - I've pulled it in now - back to the laptop CI now.
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            Tested with 2.6 and master.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! Tested with 2.6 and master.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Awesome thanks Michael.

            Show
            samhemelryk Sam Hemelryk added a comment - Awesome thanks Michael.
            Hide
            rtcn2 Rod Norfor added a comment -

            Thanks everyone for finding a solution.

            Show
            rtcn2 Rod Norfor added a comment - Thanks everyone for finding a solution.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Closing this as fixed, many thanks for your effort!

            Some people think architecture is
            about the genius sketch; I don't.
            Great architecture is a collaboration
            among a lot of people
            over a long period of time.

            – Joshua Prince-Ramus

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Closing this as fixed, many thanks for your effort! Some people think architecture is about the genius sketch; I don't. Great architecture is a collaboration among a lot of people over a long period of time. – Joshua Prince-Ramus
            Hide
            marina Marina Glancy added a comment -

            I just had the similar failure on mysql master:

            There was 1 failure:
             
            1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass), array('/home/marina/repositories/mysql_master/testdata/temp/dir1', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_1', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_1/dir1_1_1', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_1/dir1_1_1/file1_1_1_2', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_2', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_2/file1_1_2_2', '/home/marina/repositories/mysql_master/testdata/temp/dir2', '/home/marina/repositories/mysql_master/testdata/temp/dir4', '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1', '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1/dir4_1_1', '/home/marina/repositories/mysql_master/testdata/temp/file1'))
            Failed asserting that two arrays are equal.
            --- Expected
            +++ Actual
            @@ @@
                 5 => '/home/marina/repositories/mys..._1_2_2'
            -    6 => '/home/marina/repositories/mysql_master/testdata/temp/dir2'
            -    7 => '/home/marina/repositories/mysql_master/testdata/temp/dir4'
            -    8 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1'
            -    9 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1/dir4_1_1'
            -    10 => '/home/marina/repositories/mys.../file1'
            +    6 => '/home/marina/repositories/mysql_master/testdata/temp/dir4'
            +    7 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1'
            +    8 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1/dir4_1_1'
            +    9 => '/home/marina/repositories/mysql_master/testdata/temp/file1'
             )
             
            /home/marina/repositories/mysql_master/moodle/lib/tests/cronlib_test.php:179
            /home/marina/repositories/mysql_master/moodle/lib/phpunit/classes/basic_testcase.php:64
             
            To re-run:
             vendor/bin/phpunit cronlib_testcase lib/tests/cronlib_test.php
            

            I re-run it separately, it passed. I'm re-running the whole suite - it passed.

            But still it looks like we have a random failure now

            Show
            marina Marina Glancy added a comment - I just had the similar failure on mysql master: There was 1 failure:   1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass), array('/home/marina/repositories/mysql_master/testdata/temp/dir1', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_1', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_1/dir1_1_1', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_1/dir1_1_1/file1_1_1_2', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_2', '/home/marina/repositories/mysql_master/testdata/temp/dir1/dir1_2/file1_1_2_2', '/home/marina/repositories/mysql_master/testdata/temp/dir2', '/home/marina/repositories/mysql_master/testdata/temp/dir4', '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1', '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1/dir4_1_1', '/home/marina/repositories/mysql_master/testdata/temp/file1')) Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ 5 => '/home/marina/repositories/mys..._1_2_2' - 6 => '/home/marina/repositories/mysql_master/testdata/temp/dir2' - 7 => '/home/marina/repositories/mysql_master/testdata/temp/dir4' - 8 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1' - 9 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1/dir4_1_1' - 10 => '/home/marina/repositories/mys.../file1' + 6 => '/home/marina/repositories/mysql_master/testdata/temp/dir4' + 7 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1' + 8 => '/home/marina/repositories/mysql_master/testdata/temp/dir4/dir4_1/dir4_1_1' + 9 => '/home/marina/repositories/mysql_master/testdata/temp/file1' )   /home/marina/repositories/mysql_master/moodle/lib/tests/cronlib_test.php:179 /home/marina/repositories/mysql_master/moodle/lib/phpunit/classes/basic_testcase.php:64   To re-run: vendor/bin/phpunit cronlib_testcase lib/tests/cronlib_test.php I re-run it separately, it passed. I'm re-running the whole suite - it passed. But still it looks like we have a random failure now
            Hide
            nigelccatalyst Nigel Cunningham added a comment -

            I've been able to reliably reproduce this bug with Totara 2.6.10 (based on Moodle 2.6.5), and have a patch that does fix the issue.

            There are two problems.

            First, the same timestamp is not used throughout testing, so that failures can happen because a test takes too long to run. The patch I will seek to add fixes this by storing the result of time() once and passing it in a new optional parameter to cron_delete_from_temp so that its tests can use that fixed timestamp and not be subject to variations in the time taken to run tests.

            Secondly, in the week after daylight saving starts or ends (this week in Oz, which is why I found it!), the number of seconds to the same time last week is not 7 * 24 * 60 * 60. The $lastweekstime variable therefore needs to calculate the difference between strtotime('-1 week') and time(), and use that value.

            With both of these changes, I've reliably and successfully run the unit test.

            I can't see how to reopen the bug above so if I fail to that or to submit the patch, I'll open a new bug and seek to link to it from here.

            Show
            nigelccatalyst Nigel Cunningham added a comment - I've been able to reliably reproduce this bug with Totara 2.6.10 (based on Moodle 2.6.5), and have a patch that does fix the issue. There are two problems. First, the same timestamp is not used throughout testing, so that failures can happen because a test takes too long to run. The patch I will seek to add fixes this by storing the result of time() once and passing it in a new optional parameter to cron_delete_from_temp so that its tests can use that fixed timestamp and not be subject to variations in the time taken to run tests. Secondly, in the week after daylight saving starts or ends (this week in Oz, which is why I found it!), the number of seconds to the same time last week is not 7 * 24 * 60 * 60. The $lastweekstime variable therefore needs to calculate the difference between strtotime('-1 week') and time(), and use that value. With both of these changes, I've reliably and successfully run the unit test. I can't see how to reopen the bug above so if I fail to that or to submit the patch, I'll open a new bug and seek to link to it from here.
            Hide
            nigelccatalyst Nigel Cunningham added a comment -

            Patch fixing PHP unit testing issues reported in bug 45654.

            Show
            nigelccatalyst Nigel Cunningham added a comment - Patch fixing PHP unit testing issues reported in bug 45654.
            Hide
            marina Marina Glancy added a comment -

            Linking to MDL-47572 reported today about the related random unittest failure

            Show
            marina Marina Glancy added a comment - Linking to MDL-47572 reported today about the related random unittest failure

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14