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

Testing sites are cleaning all test dataroot contents

    Details

    • Testing Instructions:
      Hide

      Setup behat (init)
      a) Do not run any test. Check that in the behat dataroot you see a file called originaldatafiles.json. Check that filedir contains multiple folder (not just da folder)
      b) run a test. At the end of the test, check that dataroot_behat and dataroot_behat/filedir contain the same folders/files as in a).
      c) drop the behat database with util.php --drop. Check that originaldatafiles.json and filedir has been deleted.
      d) rerun init. Check that dataroot_behat and dataroot_behat/filedir contain the same folders/files as in a).

      Setup phpunit(init)
      a) Do not run any test. Check that in the phpunitdataroot you see a file called originaldatafiles.json. Check that filedir contains multiple folder (not just da folder)
      b) run a test. At the end of the test, check that dataroot_phpunit and dataroot_phpunit/filedir contain the same folders/files as in a).
      c) drop the phpunitdatabase with util.php --drop. Check that originaldatafiles.json and filedir has been deleted.
      d) rerun init. Check that dataroot_phpunit and dataroot_phpunit/filedir contain the same folders/files as in a).

      Last test
      a) clone a new site but don't install it.
      b) delete the mod/assign/feedback/editpdf plugin
      c) install the site
      d) run a behat test. It should not crash.

      Finally, run vendor/bin/phpunit core_test_util_testcase

      Show
      Setup behat (init) a) Do not run any test. Check that in the behat dataroot you see a file called originaldatafiles.json. Check that filedir contains multiple folder (not just da folder) b) run a test. At the end of the test, check that dataroot_behat and dataroot_behat/filedir contain the same folders/files as in a). c) drop the behat database with util.php --drop. Check that originaldatafiles.json and filedir has been deleted. d) rerun init. Check that dataroot_behat and dataroot_behat/filedir contain the same folders/files as in a). Setup phpunit(init) a) Do not run any test. Check that in the phpunitdataroot you see a file called originaldatafiles.json. Check that filedir contains multiple folder (not just da folder) b) run a test. At the end of the test, check that dataroot_phpunit and dataroot_phpunit/filedir contain the same folders/files as in a). c) drop the phpunitdatabase with util.php --drop. Check that originaldatafiles.json and filedir has been deleted. d) rerun init. Check that dataroot_phpunit and dataroot_phpunit/filedir contain the same folders/files as in a). Last test a) clone a new site but don't install it. b) delete the mod/assign/feedback/editpdf plugin c) install the site d) run a behat test. It should not crash. Finally, run vendor/bin/phpunit core_test_util_testcase
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.6 Branch:
    • Pull Master Branch:
      MDL-43266-master-nomerge5
    • Sprint:
      FRONTEND Sprint 10
    • Story Points (Obsolete):
      13
    • Sprint:
      FRONTEND Sprint 10

      Description

      Edit PDF plugin is adding files to dataroot/filedir during the moodle installation process, the same followed by phpunit and behat. When phpunit and behat resets the dataroot we clean all the contents of filedir so the files are still in the database but not in dataroot, accessing the Edit PDF admin settings page results in a fatal error when looking for the files that are supposed to be there. The files created during the moodle installation should remain in the database after the reset.

      As a solution I propose to store a list of the dataroot files present after the installation, and avoid cleaning those files when resetting the dataroot.

      Adding also 2.5 as affected branch as 3rd party plugins adding files during the installation processes are also affected.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Taking the issue as it's blocking MDL-42025. I added FRONTEND for fixVersion as I'm fixing it during the FRONTEND sprint.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Taking the issue as it's blocking MDL-42025 . I added FRONTEND for fixVersion as I'm fixing it during the FRONTEND sprint.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            I fixed the problem for behat. Sent for peer-review.

            About PHPunit: David, it is not entirely similar. I did try to fix this problem on PHPunit as you suggested. The problem is that when we run a test in PHPunit the dataroot/filedir gets deleted at the end, contrarily to Behat. I checked the code but there is reset calls in many places.

            Petr maybe you want to have a look and provide some guidance for PHPunit? What's your thoughts about the best solution?

            Show
            jerome Jérôme Mouneyrac added a comment - - edited I fixed the problem for behat. Sent for peer-review. About PHPunit: David, it is not entirely similar. I did try to fix this problem on PHPunit as you suggested. The problem is that when we run a test in PHPunit the dataroot/filedir gets deleted at the end, contrarily to Behat. I checked the code but there is reset calls in many places. Petr maybe you want to have a look and provide some guidance for PHPunit? What's your thoughts about the best solution?
            Hide
            andyjdavis Andrew Davis added a comment -

            Am I correct in saying that the behat and phpunit tests are clearing $CFG->dataroot? That seems inherently wrong. Shouldn't they be limited to $CFG->phpunit_dataroot and $CFG->behat_dataroot?

            Show
            andyjdavis Andrew Davis added a comment - Am I correct in saying that the behat and phpunit tests are clearing $CFG->dataroot? That seems inherently wrong. Shouldn't they be limited to $CFG->phpunit_dataroot and $CFG->behat_dataroot?
            Hide
            skodak Petr Skoda added a comment -

            the behat and phpunit init code replaces the value of $CFG->dataroot...

            Show
            skodak Petr Skoda added a comment - the behat and phpunit init code replaces the value of $CFG->dataroot...
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I'm in holidays by tomorrow so you can unassign it from me if someone take it over. If you take it over that would be nice to work from my commit Cheers.

            Show
            jerome Jérôme Mouneyrac added a comment - I'm in holidays by tomorrow so you can unassign it from me if someone take it over. If you take it over that would be nice to work from my commit Cheers.
            Hide
            andyjdavis Andrew Davis added a comment -

            Peer review re-assigned to David at Jerome's request.

            Show
            andyjdavis Andrew Davis added a comment - Peer review re-assigned to David at Jerome's request.
            Hide
            damyon Damyon Wiese added a comment -

            Ping for David to peer review this just because this is blocking an issue in the frontend sprint. (The patch looks OK to me).

            Show
            damyon Damyon Wiese added a comment - Ping for David to peer review this just because this is blocking an issue in the frontend sprint. (The patch looks OK to me).
            Hide
            dmonllao David Monllaó added a comment -

            The patch looks good Jerome; maybe I'm missing something, but about phpunit, but would not be as easy as calling self::save_original_data_files(); also after install_cli_database(), in phpunit_util::install_site()? Any comment Petr Skoda?

            Only 2 things to comment about the patch, but up to you, they are personal preferences.

            • I would not name an attribute $datarootlistjson, the file name gives more info about it's contents than the attr name, it is just my preference.
            • I would not send $childclassname as an argument and I would create a class attr or get it again in skip_original_data_files() again with self::get_framework() . '_util';, with the argument it seems that is "something" that the method needs, and can be perfectly obtained accessing the class contents; anyway, something minor and personal preference again.
            Show
            dmonllao David Monllaó added a comment - The patch looks good Jerome; maybe I'm missing something, but about phpunit, but would not be as easy as calling self::save_original_data_files(); also after install_cli_database(), in phpunit_util::install_site()? Any comment Petr Skoda ? Only 2 things to comment about the patch, but up to you, they are personal preferences. I would not name an attribute $datarootlistjson, the file name gives more info about it's contents than the attr name, it is just my preference. I would not send $childclassname as an argument and I would create a class attr or get it again in skip_original_data_files() again with self::get_framework() . '_util'; , with the argument it seems that is "something" that the method needs, and can be perfectly obtained accessing the class contents; anyway, something minor and personal preference again.
            Hide
            dmonllao David Monllaó added a comment -

            (I forgot to change status)

            Show
            dmonllao David Monllaó added a comment - (I forgot to change status)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi Damyon/Andrew,
            if you have time can you take over this issue? If you don't have time you can put the three issues (editpdf behat + the two behat ones) out of the sprint and I'll take care of them when I'm back.
            Thank you.

            PS: thanks David for your review.

            Show
            jerome Jérôme Mouneyrac added a comment - Hi Damyon/Andrew, if you have time can you take over this issue? If you don't have time you can put the three issues (editpdf behat + the two behat ones) out of the sprint and I'll take care of them when I'm back. Thank you. PS: thanks David for your review.
            Hide
            salvetore Michael de Raadt added a comment -

            Carrying over to the next sprint. Jerome will return next week.

            Show
            salvetore Michael de Raadt added a comment - Carrying over to the next sprint. Jerome will return next week.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            David, I added two commits (one for behat and another one for phpunit - it actually works the same in behat and phpunit, I fixed a problem with the reset function in my second commit). I didn't change the already existing $childclassname.

            Show
            jerome Jérôme Mouneyrac added a comment - David, I added two commits (one for behat and another one for phpunit - it actually works the same in behat and phpunit, I fixed a problem with the reset function in my second commit). I didn't change the already existing $childclassname.
            Hide
            dmonllao David Monllaó added a comment -

            Ok, as you prefer, the $datarootlistjson and $childclassname are personal preferences. Feel free to squash the commits before sending to integration.

            Show
            dmonllao David Monllaó added a comment - Ok, as you prefer, the $datarootlistjson and $childclassname are personal preferences. Feel free to squash the commits before sending to integration.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Jerome - this looks like a good patch. There are 2 things to look at though -

            1. Very minor - phpdocs for skip_original_data_files is missing a type for the param (picked up by automated ci checks).

            2. This needs a unit test - something like - count files in dataroot, add a Moodle file, resetAllData, count files in dataroot and verify the number is the same.

            Can you please add a test and fix the phpdocs?

            Thanks!

            Show
            damyon Damyon Wiese added a comment - Thanks Jerome - this looks like a good patch. There are 2 things to look at though - 1. Very minor - phpdocs for skip_original_data_files is missing a type for the param (picked up by automated ci checks). 2. This needs a unit test - something like - count files in dataroot, add a Moodle file, resetAllData, count files in dataroot and verify the number is the same. Can you please add a test and fix the phpdocs? Thanks!
            Hide
            damyon Damyon Wiese added a comment -

            Hi Jerome,

            Reopening for this week. Please address these comments and resubmit.

            Thanks!

            Show
            damyon Damyon Wiese added a comment - Hi Jerome, Reopening for this week. Please address these comments and resubmit. Thanks!
            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 -

            I sent it back to peer-review David. You just need to peer-review the last commit (I added a unit test).

            Show
            jerome Jérôme Mouneyrac added a comment - I sent it back to peer-review David. You just need to peer-review the last commit (I added a unit test).
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43266

            • Remote repository: git://github.com/mouneyrac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43266 Remote repository: git://github.com/mouneyrac/moodle.git Remote branch MDL-43266 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/746 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/746/artifact/work/smurf.html Remote branch MDL-43266 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/747 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/747/artifact/work/smurf.html Remote branch MDL-43266 -master-nomerge to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/748 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/748/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43266

            • Remote repository: git://github.com/mouneyrac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43266 Remote repository: git://github.com/mouneyrac/moodle.git Remote branch MDL-43266 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/752 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/752/artifact/work/smurf.html Remote branch MDL-43266 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/753 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/753/artifact/work/smurf.html Remote branch MDL-43266 -master-nomerge2 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/754 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/754/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43266

            • Remote repository: git://github.com/mouneyrac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43266 Remote repository: git://github.com/mouneyrac/moodle.git Remote branch MDL-43266 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/755 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/755/artifact/work/smurf.html Remote branch MDL-43266 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/756 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/756/artifact/work/smurf.html Remote branch MDL-43266 -master-nomerge2 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/757 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/757/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43266

            • Remote repository: git://github.com/mouneyrac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43266 Remote repository: git://github.com/mouneyrac/moodle.git Remote branch MDL-43266 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/759 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/759/artifact/work/smurf.html Remote branch MDL-43266 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/760 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/760/artifact/work/smurf.html Remote branch MDL-43266 -master-nomerge2 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/761 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/761/artifact/work/smurf.html
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Sent back to peer-review, cibot didn't detect any issue anymore

            Show
            jerome Jérôme Mouneyrac added a comment - Sent back to peer-review, cibot didn't detect any issue anymore
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Jerome, adding a few comments

            Show
            dmonllao David Monllaó added a comment - Thanks Jerome, adding a few comments https://github.com/mouneyrac/moodle/commit/7e90f9d96c5992d7ecb1ab8f0fdebd785f21155d#diff-cff35b5578d18807e2edd39d64b1fcf1R110 already commented personally, there may be other tests with the need to write files and I wonder if they use another common folder or something like that, that would be better to use than $CFG->phpunit_dataroot . '/mockdataroot', which could interfere with phpunit framework files and dirs can file_put_contents() be used rather than f [open|write|close] ()? there is no big difference and there is no point on changing this if you don't have to rebase/amend because of other points, but it usually helps with readablility Just pointing out again the vars names, while reviewing this I'm still confused by what the vars contains and I need to track the vars origin https://github.com/mouneyrac/moodle/commit/7e90f9d96c5992d7ecb1ab8f0fdebd785f21155d#diff-cff35b5578d18807e2edd39d64b1fcf1R74 mmm, not sure about this couple of comments, they seems contradictory https://github.com/mouneyrac/moodle/commit/7e90f9d96c5992d7ecb1ab8f0fdebd785f21155d#diff-b382aeb794481a200b12f1273dde5e3cR38 Not 100% sure, but maybe it needs to finish with a dot (sorry for the picky comment) Now that I think of, if the checker hasn't complained it should be ok
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Sent to integration.

            Show
            jerome Jérôme Mouneyrac added a comment - Sent to integration.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Uhm...

            If I'm not wrong... this will fail if you try to install a system not having any plugin adding files to filedir.

            save_original_data_files() seems to, blindly, assume that filedir exists.

            Just try it against 25_STABLE or against a 26/27 site with "mod/assign/feedback/editpdf" deleted. Boom!

            Once fixed, please cover that case in testing instructions, reopening for a final round, other than that I think it looks perfect (the only tiny bit I don't fully agree is the new get_dataroot() method, but it does not break anything apparently, so np).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Uhm... If I'm not wrong... this will fail if you try to install a system not having any plugin adding files to filedir. save_original_data_files() seems to, blindly, assume that filedir exists. Just try it against 25_STABLE or against a 26/27 site with "mod/assign/feedback/editpdf" deleted. Boom! Once fixed, please cover that case in testing instructions, reopening for a final round, other than that I think it looks perfect (the only tiny bit I don't fully agree is the new get_dataroot() method, but it does not break anything apparently, so np). Ciao
            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 I fixed it and verified it works on master / 2.5. Resending to integration.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Eloy I fixed it and verified it works on master / 2.5. Resending to integration.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Looks perfect, integrated (25, 26 and master), thanks!

            PS: Just a random thought, this schema works ok while we don't manipulate those "original files" in tests. I hope we won't need to do that ever or we'll need to change current skip by save and reset.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Looks perfect, integrated (25, 26 and master), thanks! PS: Just a random thought, this schema works ok while we don't manipulate those "original files" in tests. I hope we won't need to do that ever or we'll need to change current skip by save and reset.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sorry, Jerome, but I'm getting rid of these commits from integration.git:

            1) It has caused all branches in the 2 CI servers to start failing with hundreds of failures/exceptions. Here there is the raw output for master execution:

            http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(master)/2407/consoleText

            2) I've spent some time debugging the execution, to see if some quick fix was applicable and have detected some things to fix:

            • ::$datarootskiponreset accumulates and accumulates files all the time, never stopping (duplicates the same group all the time).
            • For the very first tests in the complete suite (basic tests) it seems to run ok, but once the very first advanced/database test begins, writes become somehow out of control.

            I'm afraid I've not gone deeper/more detailed with this, but the results are sooo bad, that this cannot be kept @ integration.git in its current status.

            So reopening this and rewriting git history.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sorry, Jerome, but I'm getting rid of these commits from integration.git: 1) It has caused all branches in the 2 CI servers to start failing with hundreds of failures/exceptions. Here there is the raw output for master execution: http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(master)/2407/consoleText 2) I've spent some time debugging the execution, to see if some quick fix was applicable and have detected some things to fix: ::$datarootskiponreset accumulates and accumulates files all the time, never stopping (duplicates the same group all the time). For the very first tests in the complete suite (basic tests) it seems to run ok, but once the very first advanced/database test begins, writes become somehow out of control. I'm afraid I've not gone deeper/more detailed with this, but the results are sooo bad, that this cannot be kept @ integration.git in its current status. So reopening this and rewriting git history.
            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 - - edited

            Sending for peer-review to Eloy, I just made the fix on master branch (I'll merge all commits in one and I'll make them on 2.6/2.5 if the patch is validated).

            What I did:
            a) I removed the phpunit test that was faulty and caused bg mess in 1).
            b) I fixed 2).

            About the PHPunit test, it is more complex than expected as it's basically a phpunit test that test the phpunit Moodle framework. I suggest we create an issue for that if necessary. At the beginning I just wanted to add a behat test for editpdf (MDL-42025) and I ended up to:

            • add support for filemanager in admin pages into the behat framework
            • fix behat framework bug that delete the files added during the install process from moodledata.
            • same fix for phpunit
            • work on a phpunit test to test phpunit framework (in order to check my changes)

            I think I went a bit away from writing a simple behat test and the allocated dev resource for the initial behat test issue. I would prefer either Petr (PHPunit) or David (Behat) to work on a proper unit test to test the test frameworks themself.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Sending for peer-review to Eloy, I just made the fix on master branch (I'll merge all commits in one and I'll make them on 2.6/2.5 if the patch is validated). What I did: a) I removed the phpunit test that was faulty and caused bg mess in 1). b) I fixed 2). About the PHPunit test, it is more complex than expected as it's basically a phpunit test that test the phpunit Moodle framework. I suggest we create an issue for that if necessary. At the beginning I just wanted to add a behat test for editpdf ( MDL-42025 ) and I ended up to: add support for filemanager in admin pages into the behat framework fix behat framework bug that delete the files added during the install process from moodledata. same fix for phpunit work on a phpunit test to test phpunit framework (in order to check my changes) I think I went a bit away from writing a simple behat test and the allocated dev resource for the initial behat test issue. I would prefer either Petr (PHPunit) or David (Behat) to work on a proper unit test to test the test frameworks themself.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43266

            • Remote repository: git://github.com/mouneyrac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43266 Remote repository: git://github.com/mouneyrac/moodle.git Remote branch MDL-43266 -master-nomerge4 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1572 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1572/artifact/work/smurf.html
            Hide
            jerome Jérôme Mouneyrac added a comment -

            The last commit of https://github.com/mouneyrac/moodle/compare/692d247...MDL-43266-master-nomerge4 include the only changes since Eloy's last review (see my previous comment about the decision I took). Sending to integration.

            Show
            jerome Jérôme Mouneyrac added a comment - The last commit of https://github.com/mouneyrac/moodle/compare/692d247...MDL-43266-master-nomerge4 include the only changes since Eloy's last review (see my previous comment about the decision I took). Sending to integration.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            reopening for a bit, I may have detected an issue with the download file phpunit test failing...

            Show
            jerome Jérôme Mouneyrac added a comment - reopening for a bit, I may have detected an issue with the download file phpunit test failing...
            Hide
            jerome Jérôme Mouneyrac added a comment -

            After retesting, it's all good, it was a confusion between 2.5 and master. I retested both 2.5 and master they both produce their exact same specific phpunit fails when running phpunit with and without this issue patch.

            Show
            jerome Jérôme Mouneyrac added a comment - After retesting, it's all good, it was a confusion between 2.5 and master. I retested both 2.5 and master they both produce their exact same specific phpunit fails when running phpunit with and without this issue patch.
            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
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Jerome,

            some small comments:

            1) I just realized that there is a possible "$CFG->filedir" setting that allows Moodle to move away from dataroot/filedir to any other path. And your patch is ignoring that possibility completely. In fact... the code is ignoring any of those $CFG->tempdir/localcache/cache variables that allow those subdirectories to be elsewhere. Can you confirm if phpunit/behat observe those settings or if they are 100% ignored?

            2) Which is the point of adding these 2 entries to the list. Do them any real difference. Looking to code I'm not able to find it, but sure there is a good reason, can you clarify, plz:

            +            $listfiles[] = 'filedir/.';
            +            $listfiles[] = 'filedir/..';

            3) It seems that the save_original_data_files() method returns all directories twice, not critical at all but.

            I think that, if we can confirm that 1) is not a problem, 2) has some explanation and 3) is fixed, this can land. Holding it open for some hours.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Jerome, some small comments: 1) I just realized that there is a possible "$CFG->filedir" setting that allows Moodle to move away from dataroot/filedir to any other path. And your patch is ignoring that possibility completely. In fact... the code is ignoring any of those $CFG->tempdir/localcache/cache variables that allow those subdirectories to be elsewhere. Can you confirm if phpunit/behat observe those settings or if they are 100% ignored? 2) Which is the point of adding these 2 entries to the list. Do them any real difference. Looking to code I'm not able to find it, but sure there is a good reason, can you clarify, plz: + $listfiles[] = 'filedir/.'; + $listfiles[] = 'filedir/..'; 3) It seems that the save_original_data_files() method returns all directories twice, not critical at all but. I think that, if we can confirm that 1) is not a problem, 2) has some explanation and 3) is fixed, this can land. Holding it open for some hours. Ciao
            Hide
            dmonllao David Monllaó added a comment -

            Hi, I'm not sure if Jerome will reply so commenting what I can:

            1) In phpunit case it can be ignored (https://github.com/moodle/moodle/blob/master/lib/phpunit/bootstrap.php#L181), in behat we also white list the $CFG attrs we allow (https://github.com/moodle/moodle/blob/master/lib/behat/lib.php#L154) but we allow users to specify extra allowed attributes (https://github.com/moodle/moodle/blob/master/config-dist.php#L668-L672) so, even though $CFG->behat_extraallowedsettings is not meant to be used to set vars like filedir, somebody could do it, so we should consider this possibility...

            2) For what I see reading the code should be because of opendir() returning . and .. (https://github.com/mouneyrac/moodle/compare/692d247...MDL-43266-master-nomerge5#diff-b382aeb794481a200b12f1273dde5e3cR658)

            Show
            dmonllao David Monllaó added a comment - Hi, I'm not sure if Jerome will reply so commenting what I can: 1) In phpunit case it can be ignored ( https://github.com/moodle/moodle/blob/master/lib/phpunit/bootstrap.php#L181 ), in behat we also white list the $CFG attrs we allow ( https://github.com/moodle/moodle/blob/master/lib/behat/lib.php#L154 ) but we allow users to specify extra allowed attributes ( https://github.com/moodle/moodle/blob/master/config-dist.php#L668-L672 ) so, even though $CFG->behat_extraallowedsettings is not meant to be used to set vars like filedir, somebody could do it, so we should consider this possibility... 2) For what I see reading the code should be because of opendir() returning . and .. ( https://github.com/mouneyrac/moodle/compare/692d247...MDL-43266-master-nomerge5#diff-b382aeb794481a200b12f1273dde5e3cR658 )
            Hide
            dmonllao David Monllaó added a comment -

            Eloy, adding pull branches fixing 3 in case you are interested in pulling the issue. I've been checking testing_util::skip_original_data_files() code in case there is any problem encoding json associative arrays rather than arrays but for what I see array_merge() will renumber the keys so all good if I'm not missing anything.

            git pull git://github.com/dmonllao/moodle.git MDL-43266_master-last-fixes
            git pull git://github.com/dmonllao/moodle.git MDL-43266_26-last-fixes
            git pull git://github.com/dmonllao/moodle.git MDL-43266_25-last-fixes

            Show
            dmonllao David Monllaó added a comment - Eloy, adding pull branches fixing 3 in case you are interested in pulling the issue. I've been checking testing_util::skip_original_data_files() code in case there is any problem encoding json associative arrays rather than arrays but for what I see array_merge() will renumber the keys so all good if I'm not missing anything. git pull git://github.com/dmonllao/moodle.git MDL-43266 _master-last-fixes git pull git://github.com/dmonllao/moodle.git MDL-43266 _26-last-fixes git pull git://github.com/dmonllao/moodle.git MDL-43266 _25-last-fixes
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Ok,

            so... if chances of using a custom CFG->filedir are low, once applied your anti-dupes commit... I think this can be integrated, so done.

            Note I've added 1 commit to fix file perms. Integrated (25, 26 & master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Ok, so... if chances of using a custom CFG->filedir are low, once applied your anti-dupes commit... I think this can be integrated, so done. Note I've added 1 commit to fix file perms. Integrated (25, 26 & master), thanks!
            Hide
            dmonllao David Monllaó added a comment -

            Maybe I'm doing something wrong, but I can not see this in integration

            Show
            dmonllao David Monllaó added a comment - Maybe I'm doing something wrong, but I can not see this in integration
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            I can't see this as well...

            Show
            rajeshtaneja Rajesh Taneja added a comment - I can't see this as well...
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Stopping test as this needs to go back for integration.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Stopping test as this needs to go back for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            Eloy - waiting to see what you are doing with this issue - it doesn't look like its been integrated and was integrated after the testing deadline, so I guess your plan was to self-test.

            Show
            poltawski Dan Poltawski added a comment - Eloy - waiting to see what you are doing with this issue - it doesn't look like its been integrated and was integrated after the testing deadline, so I guess your plan was to self-test.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            oh, my, sorry. I forgot to push this to integration.git. Sorry!

            I'm running all tests here locally and will push if they pass. Grrr, again, sorry.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - oh, my, sorry. I forgot to push this to integration.git. Sorry! I'm running all tests here locally and will push if they pass. Grrr, again, sorry. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Oh, David... if I'm not wrong making them associative, forces json stuff to handle it as an object, not as an array... so later it fails in array expected operations:

            Fatal error: Cannot use object of type stdClass as array in lib/testing/classes/util.php on line 844

            So, I'm going to add one more commit, using array_values(), to kill the keys before json encoding...

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Oh, David... if I'm not wrong making them associative, forces json stuff to handle it as an object, not as an array... so later it fails in array expected operations: Fatal error: Cannot use object of type stdClass as array in lib/testing/classes/util.php on line 844 So, I'm going to add one more commit, using array_values(), to kill the keys before json encoding... Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (25, 26 and master), and tested under 25 and master. All went fine.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (25, 26 and master), and tested under 25 and master. All went fine.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The price of success is hard work,
            dedication to the job at hand,
            and the determination that whether we win or lose,
            we have applied the best of ourselves to the task at hand.

            Vince Lombardi

            This is now part of Moodle, your favorite non-frameworkial LMS, LOL. Thanks, closing!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The price of success is hard work, dedication to the job at hand, and the determination that whether we win or lose, we have applied the best of ourselves to the task at hand. Vince Lombardi This is now part of Moodle, your favorite non-frameworkial LMS, LOL. Thanks, closing! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14

                  Agile