Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4
    • Fix Version/s: 2.6
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide

      (Note: There are unit tests which cover this feature.)

      1. In the Administration menu under Development, choose 'Make test course'.
      2. Select the 'XS' size option.
      3. Enter a shortname of a course that already exists on your system.
      4. Submit the form.

      EXPECTED: The form shows an error next to the shortname because it already exists.

      5. Change the shortname to an unused one and submit the form.

      EXPECTED: The page shows progress as a bullet list as it creates a small course.

      6. Click the Continue link to look at the course.

      EXPECTED: The course has a Page, a Forum (with a discussion in it), an assigned user (who posted the discussion), and two File resources.

      7. Return to the tool form. This time, select an M course and continue through the process.

      EXPECTED: The resulting course should now be significantly larger with lots of Pages, several large File resources, 100 sections, etc.

      NOTE: I have used this tool successfully up to the 'XL' option, but the larger options take a really long time, so I didn't include them in the test script.

      Show
      (Note: There are unit tests which cover this feature.) 1. In the Administration menu under Development, choose 'Make test course'. 2. Select the 'XS' size option. 3. Enter a shortname of a course that already exists on your system. 4. Submit the form. EXPECTED: The form shows an error next to the shortname because it already exists. 5. Change the shortname to an unused one and submit the form. EXPECTED: The page shows progress as a bullet list as it creates a small course. 6. Click the Continue link to look at the course. EXPECTED: The course has a Page, a Forum (with a discussion in it), an assigned user (who posted the discussion), and two File resources. 7. Return to the tool form. This time, select an M course and continue through the process. EXPECTED: The resulting course should now be significantly larger with lots of Pages, several large File resources, 100 sections, etc. NOTE: I have used this tool successfully up to the 'XL' option, but the larger options take a really long time, so I didn't include them in the test script.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-38197-master
    • Rank:
      48032

      Description

      The fundamental cause of backup and restore problems is that the system has only been tested on small courses. It should be tested on a course designed to be larger in most aspects than courses that we expect to encounter at the OU and other institutions.

      There should be a test process or script for testing large course backup/restore. (This should probably be separate from normal unit tests because it will take too long to run. It might involve an automated script that creates the stupidly big course, and then a manual test script to run through doing backup and restore.)

      Specifically, this course should have the following features (there might be others but these are the factors we identified as critical):

      • At least 5,000 activities spread across at least 1,000 sections.
      • At least 50,000 enrolled users.
      • Includes a single resource (File) containing at least 10,000 different files totalling at least 1GB.
      • Includes a forum containing at least 1,000 discussions and 10,000 posts.
      • In total across other activities, includes different files totalling at least 15GB.

      A course of this size may well not backup/restore successfully with default Moodle settings; in order to make a course of this size successfully back up and restore, it will be necessary to do some or all of the following:

      • Run on a 64-bit PHP platform.
      • Increase the ‘extra memory limit’ to a large value by using a configuration option visible in admin settings.
      • Set up paths to external zip and unzip utilities.
      • Require certain minimum component versions.

      You may need to install this admin tool as part of testing another issue. If you are using a version which already includes the fixes for MDL-41045 and MDL-41004, you should be able to just merge this branch into your code and then visit the Notifications page. It should now be available under the Developer menu.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          I'm assuming it's OK for me to grab this - say if not. I plan to develop this (or similar) for Moodle 2.6 as part of other backup/restore work I'm going to do.

          Show
          Sam Marshall added a comment - I'm assuming it's OK for me to grab this - say if not. I plan to develop this (or similar) for Moodle 2.6 as part of other backup/restore work I'm going to do.
          Hide
          Sam Marshall added a comment -

          I've started on this now.

          Show
          Sam Marshall added a comment - I've started on this now.
          Hide
          Sam Marshall added a comment -

          I have this nearly finished, but will update on Monday.

          I've built this as an admin tool - I think it should probably become a contrib module like codechecker, and I would like if (like codechecker) it can be hosted on Moodle HQ's GitHub rather than mine - I may not be able to maintain it in future through API changes etc, and this should be an important tool for all developers.

          Show
          Sam Marshall added a comment - I have this nearly finished, but will update on Monday. I've built this as an admin tool - I think it should probably become a contrib module like codechecker, and I would like if (like codechecker) it can be hosted on Moodle HQ's GitHub rather than mine - I may not be able to maintain it in future through API changes etc, and this should be an important tool for all developers.
          Hide
          Sam Marshall added a comment -

          Adding link to MDL-41045 as without that change, creating a large course takes an unfeasibly long time.

          Show
          Sam Marshall added a comment - Adding link to MDL-41045 as without that change, creating a large course takes an unfeasibly long time.
          Hide
          Sam Marshall added a comment -

          Adding link to MDL-41004 as this is required for the 'generator' system to be able to add file resources.

          Show
          Sam Marshall added a comment - Adding link to MDL-41004 as this is required for the 'generator' system to be able to add file resources.
          Hide
          Sam Marshall added a comment -

          This code is basically done, but I'm not submitting it for review until the blockers are dealt with.

          At present, the branch given here also includes commits for the two blockers, so if you look at the diff url, make sure to click on the single commit you want.

          Show
          Sam Marshall added a comment - This code is basically done, but I'm not submitting it for review until the blockers are dealt with. At present, the branch given here also includes commits for the two blockers, so if you look at the diff url, make sure to click on the single commit you want.
          Hide
          Sam Marshall added a comment -

          Requesting peer review.

          NOTE: It may be easier to look at the 3 separate commits in this change one at a time, as they are logically separated. The first commit adds the backup progress API including an extensive unit test; the second commit uses this to report progress through tasks and steps; the third contains the UI changes to display it.

          Here is an overview of the differences users will see from this feature when doing a backup or restore (description is for backup but the same applies to restore):

          1. Previously, backup step '4. Perform backup', though shown in the backup sequence at the top of the page, never actually became current. Now, when you get to that point, the step becomes current.

          2. Previously there was a long wait between step 3 and 5 while it carries out the entire backup. Now you see a progress bar moving across while waiting.
          NOTE: There is still some jerkiness/long waits in the progress bar - I hope to improve this in later changes.
          NOTE: Until MDL-41050 is fixed, the progress bar and the wibbler bar (thin box below it) do not quite line up.

          3. Once step 4 completes the step 5 page ('Complete') immediately appears (this is actually in the same HTML page).

          Overall this change improves the user interface as well as making it potentially possible (with other changes) to backup large courses.

          Note that there is no change to any of the backup/restore logic. The only change is a new API to obtain the progress reporting object, and some added calls to this API. So this change should not break any backup/restore behaviour.

          Show
          Sam Marshall added a comment - Requesting peer review. NOTE: It may be easier to look at the 3 separate commits in this change one at a time, as they are logically separated. The first commit adds the backup progress API including an extensive unit test; the second commit uses this to report progress through tasks and steps; the third contains the UI changes to display it. Here is an overview of the differences users will see from this feature when doing a backup or restore (description is for backup but the same applies to restore): 1. Previously, backup step '4. Perform backup', though shown in the backup sequence at the top of the page, never actually became current. Now, when you get to that point, the step becomes current. 2. Previously there was a long wait between step 3 and 5 while it carries out the entire backup. Now you see a progress bar moving across while waiting. NOTE: There is still some jerkiness/long waits in the progress bar - I hope to improve this in later changes. NOTE: Until MDL-41050 is fixed, the progress bar and the wibbler bar (thin box below it) do not quite line up. 3. Once step 4 completes the step 5 page ('Complete') immediately appears (this is actually in the same HTML page). Overall this change improves the user interface as well as making it potentially possible (with other changes) to backup large courses. Note that there is no change to any of the backup/restore logic. The only change is a new API to obtain the progress reporting object, and some added calls to this API. So this change should not break any backup/restore behaviour.
          Hide
          Sam Marshall added a comment -

          I have attached a zip of this report. I'll edit the description to put instructions about how to install this (for people to use when testing other issues).

          Show
          Sam Marshall added a comment - I have attached a zip of this report. I'll edit the description to put instructions about how to install this (for people to use when testing other issues).
          Hide
          Petr Škoda added a comment -

          Hi, why did you decide to create a new tool_makelargecourse instead of adding this functionality to the existing tool_generator? The generator is not in a good shape, but one day we need to start rebuilding it.

          Show
          Petr Škoda added a comment - Hi, why did you decide to create a new tool_makelargecourse instead of adding this functionality to the existing tool_generator? The generator is not in a good shape, but one day we need to start rebuilding it.
          Hide
          Sam Marshall added a comment -

          Petr: I did consider tool_generator first but it looked like it didn't do anything like this, didn't even use the generator backend and was too complex / too many options, so I thought it would be easier to start a new one? Basically it looked pretty useless.

          Maybe this could replace tool_generator?

          Show
          Sam Marshall added a comment - Petr: I did consider tool_generator first but it looked like it didn't do anything like this, didn't even use the generator backend and was too complex / too many options, so I thought it would be easier to start a new one? Basically it looked pretty useless. Maybe this could replace tool_generator?
          Hide
          Petr Škoda added a comment -

          you could just add it there next to the current files with own page link, the point is we can not easily remove plugins from official distribution and tool_generator seems like the best place to me

          Show
          Petr Škoda added a comment - you could just add it there next to the current files with own page link, the point is we can not easily remove plugins from official distribution and tool_generator seems like the best place to me
          Hide
          Sam Marshall added a comment -

          Petr: Good idea! Thanks. I'll modify it for this, then resubmit for peer review.

          Show
          Sam Marshall added a comment - Petr: Good idea! Thanks. I'll modify it for this, then resubmit for peer review.
          Hide
          Sam Marshall added a comment -

          Updated - I've moved all the code into tool/generator, as suggested. The existing tool/generator code is unchanged (I am not sure it actually works), but there's a link direct to this new code as 'Make test course' under developer menu.

          Show
          Sam Marshall added a comment - Updated - I've moved all the code into tool/generator, as suggested. The existing tool/generator code is unchanged (I am not sure it actually works), but there's a link direct to this new code as 'Make test course' under developer menu.
          Hide
          Frédéric Massart added a comment -

          Hi,

          I am trying to use this (zip file or git branch) tool to test MDL-39181, but it fails with the following:

          Fatal error: Class 'phpunit_util' not found in /home/fred/www/repositories/im/moodle/admin/tool/generator/classes/backend.php on line 125
          
          Call Stack:
              0.0002     667440   1. {main}() /home/fred/www/repositories/im/moodle/admin/tool/generator/maketestcourse.php:0
              1.1507   93941904   2. tool_generator_backend->make() /home/fred/www/repositories/im/moodle/admin/tool/generator/maketestcourse.php:55
          
          Show
          Frédéric Massart added a comment - Hi, I am trying to use this (zip file or git branch) tool to test MDL-39181 , but it fails with the following: Fatal error: Class 'phpunit_util' not found in /home/fred/www/repositories/im/moodle/admin/tool/generator/classes/backend.php on line 125 Call Stack: 0.0002 667440 1. {main}() /home/fred/www/repositories/im/moodle/admin/tool/generator/maketestcourse.php:0 1.1507 93941904 2. tool_generator_backend->make() /home/fred/www/repositories/im/moodle/admin/tool/generator/maketestcourse.php:55
          Hide
          Sam Marshall added a comment -

          Thanks Fred. Sorry I didn't notice this before I responded in the other issue! Looking at this now. (Don't really understand why it works on my machine, but I guess I need to add a require.)

          Show
          Sam Marshall added a comment - Thanks Fred. Sorry I didn't notice this before I responded in the other issue! Looking at this now. (Don't really understand why it works on my machine, but I guess I need to add a require.)
          Hide
          Sam Marshall added a comment -

          I've updated it to add a require_once for the file that was missing on Frederic's system, and have also tweaked a few text strings and written testing instructions. Resubmitting for review.

          Show
          Sam Marshall added a comment - I've updated it to add a require_once for the file that was missing on Frederic's system, and have also tweaked a few text strings and written testing instructions. Resubmitting for review.
          Hide
          Frédéric Massart added a comment - - edited

          +1000 to be able to run that from CLI ! Also could this flush the output buffer once in a while so that the progress is displayed?

          Show
          Frédéric Massart added a comment - - edited +1000 to be able to run that from CLI ! Also could this flush the output buffer once in a while so that the progress is displayed?
          Hide
          Sam Marshall added a comment -

          OK:

          1. It is supposed to flush the output buffer every time it prints a dot, I will check it does...
          2. I don't know how to add CLI code but I will find out, there is no reason it shouldn't work from CLI. Reopening development.

          Show
          Sam Marshall added a comment - OK: 1. It is supposed to flush the output buffer every time it prints a dot, I will check it does... 2. I don't know how to add CLI code but I will find out, there is no reason it shouldn't work from CLI. Reopening development.
          Hide
          Sam Marshall added a comment -

          Boom! CLI support, as requested. The progress display looks the same as the HTML version and it works the same. Here's an example:

          $ php admin/tool/generator/cli/maketestcourse.php --shortname=SIZE_M --size=M --bypasscheck
          * Creating course SIZE_M
          * Checking user accounts (1000)
          * Creating user accounts (101 - 1000): ...done (3.3s)
          * Enrolling users into course (1000): ....done (4.1s)
          * Creating pages (200): ..done (1.5s)
          * Creating small files (128): .....done (5.6s)
          * Creating big files (5): ..............................78.8%.......done (37.2s)
          * Creating forum (500 posts): .done (0.9s)
          * Complete (52.8s)
          

          Since a few other devs have looked at this now, I think it's probably generally OK and it would be good to get it submitted - I'll ask Tim to do a peer review.

          Show
          Sam Marshall added a comment - Boom! CLI support, as requested. The progress display looks the same as the HTML version and it works the same. Here's an example: $ php admin/tool/generator/cli/maketestcourse.php --shortname=SIZE_M --size=M --bypasscheck * Creating course SIZE_M * Checking user accounts (1000) * Creating user accounts (101 - 1000): ...done (3.3s) * Enrolling users into course (1000): ....done (4.1s) * Creating pages (200): ..done (1.5s) * Creating small files (128): .....done (5.6s) * Creating big files (5): ..............................78.8%.......done (37.2s) * Creating forum (500 posts): .done (0.9s) * Complete (52.8s) Since a few other devs have looked at this now, I think it's probably generally OK and it would be good to get it submitted - I'll ask Tim to do a peer review.
          Hide
          Sam Marshall added a comment -

          Tim, could you review this one please? It passes code checker. (By 'it' I mean my new part, not the existing legacy part of the tool.)

          Show
          Sam Marshall added a comment - Tim, could you review this one please? It passes code checker. (By 'it' I mean my new part, not the existing legacy part of the tool.)
          Hide
          Petr Škoda added a comment -

          Do not flush, instead use standard define('NO_OUTPUT_BUFFERING', true); before requiring config.php

          Show
          Petr Škoda added a comment - Do not flush, instead use standard define('NO_OUTPUT_BUFFERING', true); before requiring config.php
          Hide
          David Monllaó added a comment -

          Hi Sam,

          More comments about it; this can also be used as generator for other purposes like performance tests, so it would make sense to remove the references to backup/restore (admin/tool/generator/maketestcourse.php and admin/tool/generator/lang/en/tool_generator.php) also to open more the API or to add options would be interesting but it can be done after this gets integrated.

          The current tool_generator is not working so we should also think about what to do with it, this seems a good replacement, I'm not sure if it is worth to keep the current one as it is only adding confusion to users.

          Show
          David Monllaó added a comment - Hi Sam, More comments about it; this can also be used as generator for other purposes like performance tests, so it would make sense to remove the references to backup/restore (admin/tool/generator/maketestcourse.php and admin/tool/generator/lang/en/tool_generator.php) also to open more the API or to add options would be interesting but it can be done after this gets integrated. The current tool_generator is not working so we should also think about what to do with it, this seems a good replacement, I'm not sure if it is worth to keep the current one as it is only adding confusion to users.
          Hide
          Petr Škoda added a comment -

          Eloy told me he is using the existing generator in some CI stuff, please wait for him to comment before removing the current generator.

          Show
          Petr Škoda added a comment - Eloy told me he is using the existing generator in some CI stuff, please wait for him to comment before removing the current generator.
          Hide
          Tim Hunt added a comment -

          Overall very good. Just some minor points. If you address these, you are good for integration as far as I am concerned.

          More significant points: (Well, when I say more significant )

          1. I see NO_OUTPUT_BUFFERING turns on ob_implicit_flush. Useful, but not very discoverable.

          2. Is there any issue with using format_text for non-user input? https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L4R39. I guess not. get_formatted_help_string in weblib.php does the same.

          3. raise_memory_limit($CFG->extramemorylimit); is wrong. You should use raise_memory_limit(MEMORY_EXTRA).

          4. Unit test class name: if you called your class tool_generator_generator_testcase, and called the file tests/generator.php, then class autoloading would work (you could just type phpunit tool_generator_generator_testcase). This does not seem to be documented somewhere, but it works.

          5. get_string will accept an array or and object. You don't need the cast in https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L0R157

          6. See http://stackoverflow.com/questions/4757392/php-fast-random-string-function if you care

          7. $tempfolder = $CFG->tempdir . '/tool_generator'; check_dir_exists($tempfolder); Should use make_temp_directory instead.

          Very minor issues: (you probably don't need to fix these)

          1. I don't see the point of PHP doc comments on standard methods like public function definition(). The PHPdoc gives the external contract of the method, which is the same as in the base class. Adding a comment in the subclass is duplicated code, unless there is something significant to add about how the API works. However, if you are adding a comment, like on validation method, you must document all parameters.

          2. Just random dissing of your code design, but I would not put that for loop in form definition. I would have made a tool_generator_backend::get_size_choices method. No need for you to change that.

          3. Similarly, for validating the size in CLI, tool_generator_backend::size_for_name

          4. And is_shortname_used method would save a tiny bit of duplication.

          5. You are probably not meant to define multiple variables on a line like this: https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L0R80

          6. I remember a discussion about whether it should be exit; exit(); die; or die(); but I am afraid I don't remember what the answer is. https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L4R46

          Show
          Tim Hunt added a comment - Overall very good. Just some minor points. If you address these, you are good for integration as far as I am concerned. More significant points: (Well, when I say more significant ) 1. I see NO_OUTPUT_BUFFERING turns on ob_implicit_flush. Useful, but not very discoverable. 2. Is there any issue with using format_text for non-user input? https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L4R39 . I guess not. get_formatted_help_string in weblib.php does the same. 3. raise_memory_limit($CFG->extramemorylimit); is wrong. You should use raise_memory_limit(MEMORY_EXTRA). 4. Unit test class name: if you called your class tool_generator_generator_testcase, and called the file tests/generator.php, then class autoloading would work (you could just type phpunit tool_generator_generator_testcase). This does not seem to be documented somewhere, but it works. 5. get_string will accept an array or and object. You don't need the cast in https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L0R157 6. See http://stackoverflow.com/questions/4757392/php-fast-random-string-function if you care 7. $tempfolder = $CFG->tempdir . '/tool_generator'; check_dir_exists($tempfolder); Should use make_temp_directory instead. Very minor issues: (you probably don't need to fix these) 1. I don't see the point of PHP doc comments on standard methods like public function definition(). The PHPdoc gives the external contract of the method, which is the same as in the base class. Adding a comment in the subclass is duplicated code, unless there is something significant to add about how the API works. However, if you are adding a comment, like on validation method, you must document all parameters. 2. Just random dissing of your code design, but I would not put that for loop in form definition. I would have made a tool_generator_backend::get_size_choices method. No need for you to change that. 3. Similarly, for validating the size in CLI, tool_generator_backend::size_for_name 4. And is_shortname_used method would save a tiny bit of duplication. 5. You are probably not meant to define multiple variables on a line like this: https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L0R80 6. I remember a discussion about whether it should be exit; exit(); die; or die(); but I am afraid I don't remember what the answer is. https://github.com/sammarshallou/moodle/compare/master...MDL-38197-master#L4R46
          Hide
          Tim Hunt added a comment -
          [N] Syntax        - see comments above.
          [Y] Whitespace
          [Y] Output
          [?] Language      - see David's comment above.
          [Y] Databases
          [N] Testing       - minor issue about the class name.
          [Y] Security
          [?] Documentation - should the be mentioned somewhere in dev docs?
          [Y] Git
          [N] Sanity check  - well, the XXL option is clearly insane!
                            - more seriously, see comments above.
          
          Show
          Tim Hunt added a comment - [N] Syntax - see comments above. [Y] Whitespace [Y] Output [?] Language - see David's comment above. [Y] Databases [N] Testing - minor issue about the class name. [Y] Security [?] Documentation - should the be mentioned somewhere in dev docs? [Y] Git [N] Sanity check - well, the XXL option is clearly insane! - more seriously, see comments above.
          Hide
          Sam Marshall added a comment -

          Thanks all for review and comments! To answer some points that people raised before Tim's review:

          1. Re remove current generator - IMO there is no need to do that in this issue, which is why I left that code untouched - obviously it might make sense to remove it, but we can do that as separate issue later & check everyone is OK with it.

          2. Re adding extra features, this currently has no options other than size of course - I think that's actually quite a good thing as it means there is a simple standard thing - but even if there weren't options, it might be nice if e.g. it added a full set of modules rather than just a few types. Basically, I'd like to leave this for future enhancements.

          3. Agree with removing mentions in text/code to backup and restore as this can be used more widely, I'll do this.

          4. Re NO_OUTPUT_BUFFERING, I didn't really understand this request - I don't think this really matters as it is an admin tool only, so I will go ahead and change it, but isn't it simpler, more descriptive, and generally better to call flush() at the necessary points? The page prints a bunch of other stuff which should certainly be buffered. Manual flushing seems to me a lot more sensible than turning off buffering for the entire page. Or is there some reason why NO_OUTPUT_BUFFERING is superior like there is some situation I didn't think of where calling flush() actually breaks things? Doesn't matter, like I said I'll change it anyhow.

          Show
          Sam Marshall added a comment - Thanks all for review and comments! To answer some points that people raised before Tim's review: 1. Re remove current generator - IMO there is no need to do that in this issue, which is why I left that code untouched - obviously it might make sense to remove it, but we can do that as separate issue later & check everyone is OK with it. 2. Re adding extra features, this currently has no options other than size of course - I think that's actually quite a good thing as it means there is a simple standard thing - but even if there weren't options, it might be nice if e.g. it added a full set of modules rather than just a few types. Basically, I'd like to leave this for future enhancements. 3. Agree with removing mentions in text/code to backup and restore as this can be used more widely, I'll do this. 4. Re NO_OUTPUT_BUFFERING, I didn't really understand this request - I don't think this really matters as it is an admin tool only, so I will go ahead and change it, but isn't it simpler, more descriptive, and generally better to call flush() at the necessary points? The page prints a bunch of other stuff which should certainly be buffered. Manual flushing seems to me a lot more sensible than turning off buffering for the entire page. Or is there some reason why NO_OUTPUT_BUFFERING is superior like there is some situation I didn't think of where calling flush() actually breaks things? Doesn't matter, like I said I'll change it anyhow.
          Hide
          Sam Marshall added a comment - - edited

          Thanks to Tim for review. I have addressed all mentioned points so I think it is now OK to submit for integration.

          Changes in response to other comments:

          • Removed references to backup/restore
          • Removed flush() and added define('NO_OUTPUT_BUFFERING', true);

          Another change that I hadn't noticed:

          • I was using if (defined('CLI_SCRIPT')) - this is incorrect as it is always defined, either to true or false, the correct is if(CLI_SCRIPT); changed.

          Tim's review points (leaving out the ones that didn't suggest a change):

          3. Changed.
          4. Changed. I renamed the class to tool_generator_maketestcourse_testcase and the file to maketestcourse_test.php (you forgot the _test). Neat, this does now work with "vendor/bin/phpunit tool_generator_makelargecourse_testcase" - of course, it's actually easier to type "vendor/bin/phpunit admin/tool/generator/tests/maketestcourse_test.php" because you can use tab completion for the latter, but hey.
          5. Changed.
          6. Not changed. I read that page but don't think it's probably worth changing it - bearing in mind we don't care about cryptographic security, which is part of the point of that question, and it doesn't seem to be massively slow. The reason for randomness is just to make sure that zip doesn't compress it down to nothing - originally I had files which were like, all filled with one letter, and this didn't really stress the size of the resulting zip files. For that reason I want to use a full range of binary data not just ascii like the code in that question, or it'll compress a bunch.
          7. Changed. Neat, didn't know about that function.

          Tim's review points, part two:

          1. Changed. Agree 100% about not documenting function from base class when there is nothing to say and it follows normal pattern from API. I thought codechecker objected, but it does not.
          2. Changed.
          3. Changed.
          4. Changed.
          5. Changed. I think private variables should be listed in one line and not documented at that location, but Moodle coding practice doesn't seem to agree with me, and that doesn't work so well with IDE type detection. So I've split all those variables and added documentation for each of them.
          6. Not changed. exit(N) is used across a bunch of the CLI code so I think it's OK to use that version.

          Regarding developer docs, I have written a page here (but have not linked it from anywhere):

          http://docs.moodle.org/dev/Test_course_generator

          Show
          Sam Marshall added a comment - - edited Thanks to Tim for review. I have addressed all mentioned points so I think it is now OK to submit for integration. Changes in response to other comments: Removed references to backup/restore Removed flush() and added define('NO_OUTPUT_BUFFERING', true); Another change that I hadn't noticed: I was using if (defined('CLI_SCRIPT')) - this is incorrect as it is always defined, either to true or false, the correct is if(CLI_SCRIPT); changed. Tim's review points (leaving out the ones that didn't suggest a change): 3. Changed. 4. Changed. I renamed the class to tool_generator_maketestcourse_testcase and the file to maketestcourse_test.php (you forgot the _test). Neat, this does now work with "vendor/bin/phpunit tool_generator_makelargecourse_testcase" - of course, it's actually easier to type "vendor/bin/phpunit admin/tool/generator/tests/maketestcourse_test.php" because you can use tab completion for the latter, but hey. 5. Changed. 6. Not changed. I read that page but don't think it's probably worth changing it - bearing in mind we don't care about cryptographic security, which is part of the point of that question, and it doesn't seem to be massively slow. The reason for randomness is just to make sure that zip doesn't compress it down to nothing - originally I had files which were like, all filled with one letter, and this didn't really stress the size of the resulting zip files. For that reason I want to use a full range of binary data not just ascii like the code in that question, or it'll compress a bunch. 7. Changed. Neat, didn't know about that function. Tim's review points, part two: 1. Changed. Agree 100% about not documenting function from base class when there is nothing to say and it follows normal pattern from API. I thought codechecker objected, but it does not. 2. Changed. 3. Changed. 4. Changed. 5. Changed. I think private variables should be listed in one line and not documented at that location, but Moodle coding practice doesn't seem to agree with me, and that doesn't work so well with IDE type detection. So I've split all those variables and added documentation for each of them. 6. Not changed. exit(N) is used across a bunch of the CLI code so I think it's OK to use that version. Regarding developer docs, I have written a page here (but have not linked it from anywhere): http://docs.moodle.org/dev/Test_course_generator
          Hide
          Sam Marshall added a comment -

          Submitting for integration review.

          Show
          Sam Marshall added a comment - Submitting for integration review.
          Hide
          Dan Poltawski added a comment -

          1. Changed. Agree 100% about not documenting function from base class when there is nothing to say and it follows normal pattern from API. I thought codechecker objected, but it does not.

          I think docs checker probably does, you raised this as a coding style issue: MDLSITE-1775

          Show
          Dan Poltawski added a comment - 1. Changed. Agree 100% about not documenting function from base class when there is nothing to say and it follows normal pattern from API. I thought codechecker objected, but it does not. I think docs checker probably does, you raised this as a coding style issue: MDLSITE-1775
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          Looks good - just one tiny niggle. Surely the LIKE should be converted to $DB->sql_like()

          Show
          Dan Poltawski added a comment - Hi Sam, Looks good - just one tiny niggle. Surely the LIKE should be converted to $DB->sql_like()
          Hide
          Tim Hunt added a comment -

          Sam is on holiday this week and next week. Please can you fix this, or, I suppose, I could. (And how did I not notice that?)

          Show
          Tim Hunt added a comment - Sam is on holiday this week and next week. Please can you fix this, or, I suppose, I could. (And how did I not notice that?)
          Hide
          Dan Poltawski added a comment -

          Yep, thanks for letting me know Tim. I'll do it.

          Show
          Dan Poltawski added a comment - Yep, thanks for letting me know Tim. I'll do it.
          Hide
          Dan Poltawski added a comment -

          Integrated to master with that fix, thanks!

          Show
          Dan Poltawski added a comment - Integrated to master with that fix, thanks!
          Hide
          Dan Poltawski added a comment -

          I created MDL-41311 to remove the legacy mess of the existing code.

          Show
          Dan Poltawski added a comment - I created MDL-41311 to remove the legacy mess of the existing code.
          Hide
          Martin Dougiamas added a comment -

          I'm concerned about this tool in terms of it being used for any sort of comparisons between Moodle versions.

          Being in core means it will evolve with each branch, and I think that makes comparability between versions difficult.

          We really need to think through and define the management of this tool carefully if we're going to be using it for any performance comparisons.

          Show
          Martin Dougiamas added a comment - I'm concerned about this tool in terms of it being used for any sort of comparisons between Moodle versions. Being in core means it will evolve with each branch, and I think that makes comparability between versions difficult. We really need to think through and define the management of this tool carefully if we're going to be using it for any performance comparisons.
          Hide
          Dan Poltawski added a comment -

          As Petr said in the chat:

          the first step is to measure performance before & after individual patches, developers need some tool to create enough data for the measurement, imo the tool_generator is a good place for it

          Show
          Dan Poltawski added a comment - As Petr said in the chat: the first step is to measure performance before & after individual patches, developers need some tool to create enough data for the measurement, imo the tool_generator is a good place for it
          Hide
          Tim Hunt added a comment -

          Any tool could be changed at any time.

          We certainly need to think about and decide what is requried.

          Once that is agreed, having the tool in core means that Integrators control what changes are made to it, and it also means we can be sure that everyone is using the same version of the tool.

          Show
          Tim Hunt added a comment - Any tool could be changed at any time. We certainly need to think about and decide what is requried. Once that is agreed, having the tool in core means that Integrators control what changes are made to it, and it also means we can be sure that everyone is using the same version of the tool.
          Hide
          Frédéric Massart added a comment -

          Working as described, passing. Thanks!

          Show
          Frédéric Massart added a comment - Working as described, passing. Thanks!
          Hide
          Martin Dougiamas added a comment -

          David mentioned an idea from Eloy that helps comparability between versions which is this:

          For comparing 2.6 and 2.7 on the same hardware, you must do the data generation on 2.6 and then upgrade it to 2.7. And then for BOTH sites you use the exact same jmeter test plan generated by 2.6.

          That way you have two sites that can be legitimately compared for performance reasons. It should work most of the time because we don't change URLs much. This gets rid of a lot of my objections above.

          Show
          Martin Dougiamas added a comment - David mentioned an idea from Eloy that helps comparability between versions which is this: For comparing 2.6 and 2.7 on the same hardware, you must do the data generation on 2.6 and then upgrade it to 2.7. And then for BOTH sites you use the exact same jmeter test plan generated by 2.6. That way you have two sites that can be legitimately compared for performance reasons. It should work most of the time because we don't change URLs much. This gets rid of a lot of my objections above.
          Hide
          Damyon Wiese added a comment -

          Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

          Hurray!

          Show
          Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!
          Hide
          Frédéric Massart added a comment -

          FYI, MDL-41582.

          Show
          Frédéric Massart added a comment - FYI, MDL-41582 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: