Moodle
  1. Moodle
  2. MDL-16483

Strategy for generating and cleaning data for unit tests

    Details

    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Rank:
      1220

      Description

      This needs an urgent consensus and decision.

      We have all the tools we need to write effective unit tests for our new code. However, many parts of the code require actual database records or physical files in order to be tested properly. We need to decide which strategy will be used for cleaning up generated data, among the following:

      1. Use an alternate prefix for Database tables. This has the advantage of securing the "real" tables against actions by the unit tests. The main problem is that it is complex to keep these tables in sync with the "real" tables schema-wise, and it would be too slow to rebuild them entirely between each test
      2. Use the real tables but restrict the use of data generation and cleaning for sites where the $CFG->developmentmode variable is set in config.php (assuming that only one user will use the site). This is much simpler than 1., but can only be used on a development site, preferably freshly installed.

      In addition to deciding between real and alternate db prefix, we need to decide how the database will be "cleaned up" between each test.

      1. Complete rebuild using a dump sql file previously generated on demand. This is not really feasible because it would be horrendously slow
      2. Specifically hard-code a list of tables to be cleaned up after or before each test, some having special conditions if some records must be kept (like the users table). This is fast, but complex to set up and not very reliable. It is also difficult to maintain, as the db schema changes throughout the dev process.
      3. Before each generation of data, save in memory the highest PK number of every non-empty table (also record tables that are empty), and use this data during clean up to truncate these tables conditionally. This is much more reliable than 2., but probably slower. It also precludes any more than one concurrent user on the site, because of the risks of parallel db activity.

      Another option is available, which is more radical since it involves completely mocking the $DB object, and controlling all DB activity starting in unit tests. This puts a huge part of responsibility on the unit tests, since they have to set up return values for every get_* DB call their tested code is likely to produce. The process could be optimised using clever object orientation and abstraction, but it is a huge job nonetheless. It can however be done, and may be the strategy of choice for certain unit tests. Text or XML fixture data could easily be used in combination with this.

      This may all seem like overkill or a waste of time in this very busy time of 2.0 development, but I think we have a great opportunity to strengthen the long-term integrity of our code base, and to reduce the risks of insidious bugs causing problems in the future.

      Please comment and vote for your solution, as well as propose your own if you have any.

        Issue Links

          Activity

          Hide
          Eric Merrill added a comment -

          I vote #3

          I think that the other plans are either a ) too slow, meaning people wont use them when they should or b ) too hard to implement, meaning people wont implement them when they should...
          I think concurrent use is an ok thing to lose in this case

          Show
          Eric Merrill added a comment - I vote #3 I think that the other plans are either a ) too slow, meaning people wont use them when they should or b ) too hard to implement, meaning people wont implement them when they should... I think concurrent use is an ok thing to lose in this case
          Hide
          Penny Leach added a comment -

          Nico I was wondering about files as well... things like assignment submissions that include files. Can we make the generator generate those too? Like perhaps from a store of 'test' files?

          The portfolio code needs to test the integration between the caller (eg assignment) writing files (eg submission) and the portfolio plugin picking it up and doing something with it...

          Show
          Penny Leach added a comment - Nico I was wondering about files as well... things like assignment submissions that include files. Can we make the generator generate those too? Like perhaps from a store of 'test' files? The portfolio code needs to test the integration between the caller (eg assignment) writing files (eg submission) and the portfolio plugin picking it up and doing something with it...
          Hide
          Tim Hunt added a comment -

          I have a proposal 4, that is like 3. but more sophisticated. (Actually, I think it is inspired by something Nicolas said in HQ chat over the weekend, but I go into more detail.)

          We make a subclass of $DB (call it test_db) that:

          i) In insert_record, keeps track, for each database table, of the id of each row inserted using this object - do this either in memory, in a file, or on disc. There is a speed/reliability issue here. This list of added rows is local to the current unit test run - so it is a bit like the backupids table, effectively there will be a table with three columns testrunid, tablename and rowid.
          ii) For simple calls to update_record and delete record, we check that the row to be updated/deleted is one of the ones in our list of rows inserted during the test, and if not do an asser_fail - there is nothing we can do about the few places we do complicated SQL deletes and updates, but there are not many of those.
          iii) For simple get_record(s) calls, where we are selecting from a single table, we check that we are only returning rows that were inserted as part of the test.
          iv) There is a clean-up method (Use the destructor for this?) called at the end of the test run, which deletes all the test rows we have inserted.

          This based on the assumption that test methods should only update or delete data that they themselves have inserted. If this assumption is true, then my proposed test class would increase the power of the unit tests to check that code does not modify the wrong data. If that assumption is false, then this whole proposal is rubbish.

          So typical test code would then look like:

          // before a block of unit tests - perhaps in each test class constructor.
          setup_test_db(); // This would replace the global $DB with an instance of the test_db class.
          // create test data.

          // All the test methods, no change required.

          // After all the tests.
          cleanup_test_db();

          This will be slower than 3, but a bit safer in the case that one person is running unit tests while other people are doing things with the site, or where you yourself are doing things with the site while some slow unit tests run.

          Here are some possible extra features that could be added to the basic four above, but they would not be needed in a first release:

          v) Possibly, have it so that when a test run starts, if the place where test data ids are stored contains traced of a previous test run that crashed, it automatically cleans up the rows from the old test run? Another thing to consider if it is a good idea.

          vi) to get round the problem of complex inserts and updates in ii). Provide some assert methods on the test DB class that can be called after some complex SQL, for example assert_record_exists, or assert_record_deleted. These could be used both to check that the complicated SQL did what it was supposed to do in the database, but also be used to update the list of rowids containing test data.

          Thinking about where to log ids, my preference would be to store them in arrays in memory, so they can be checked quickly, but also write them to a simple plain text file, to enable v).

          We can also think about how often we call setup_test_db/cleanup_test_db. We could do it once for the whole test run, and that would work, or we could do it more frequently. That would be a bit slower, but would mean that bad side-effect of tests methods are identified more quickly, and are easier to localise. Actually, we can probably do the setup_test_db/cleanup_test_db in the simpletest driver methods, so the test code does not have to think about them at all.

          Show
          Tim Hunt added a comment - I have a proposal 4, that is like 3. but more sophisticated. (Actually, I think it is inspired by something Nicolas said in HQ chat over the weekend, but I go into more detail.) We make a subclass of $DB (call it test_db) that: i) In insert_record, keeps track, for each database table, of the id of each row inserted using this object - do this either in memory, in a file, or on disc. There is a speed/reliability issue here. This list of added rows is local to the current unit test run - so it is a bit like the backupids table, effectively there will be a table with three columns testrunid, tablename and rowid. ii) For simple calls to update_record and delete record, we check that the row to be updated/deleted is one of the ones in our list of rows inserted during the test, and if not do an asser_fail - there is nothing we can do about the few places we do complicated SQL deletes and updates, but there are not many of those. iii) For simple get_record(s) calls, where we are selecting from a single table, we check that we are only returning rows that were inserted as part of the test. iv) There is a clean-up method (Use the destructor for this?) called at the end of the test run, which deletes all the test rows we have inserted. This based on the assumption that test methods should only update or delete data that they themselves have inserted. If this assumption is true, then my proposed test class would increase the power of the unit tests to check that code does not modify the wrong data. If that assumption is false, then this whole proposal is rubbish. So typical test code would then look like: // before a block of unit tests - perhaps in each test class constructor. setup_test_db(); // This would replace the global $DB with an instance of the test_db class. // create test data. // All the test methods, no change required. // After all the tests. cleanup_test_db(); This will be slower than 3, but a bit safer in the case that one person is running unit tests while other people are doing things with the site, or where you yourself are doing things with the site while some slow unit tests run. Here are some possible extra features that could be added to the basic four above, but they would not be needed in a first release: v) Possibly, have it so that when a test run starts, if the place where test data ids are stored contains traced of a previous test run that crashed, it automatically cleans up the rows from the old test run? Another thing to consider if it is a good idea. vi) to get round the problem of complex inserts and updates in ii). Provide some assert methods on the test DB class that can be called after some complex SQL, for example assert_record_exists, or assert_record_deleted. These could be used both to check that the complicated SQL did what it was supposed to do in the database, but also be used to update the list of rowids containing test data. Thinking about where to log ids, my preference would be to store them in arrays in memory, so they can be checked quickly, but also write them to a simple plain text file, to enable v). We can also think about how often we call setup_test_db/cleanup_test_db. We could do it once for the whole test run, and that would work, or we could do it more frequently. That would be a bit slower, but would mean that bad side-effect of tests methods are identified more quickly, and are easier to localise. Actually, we can probably do the setup_test_db/cleanup_test_db in the simpletest driver methods, so the test code does not have to think about them at all.
          Hide
          Nicolas Connault added a comment - - edited

          After a long brainstorming session in Moodle HQ, here is the consensual decision:

          1. DB tables with alternate prefix are required before Unit tests are available.
          2. These tables are not built during install
          3. The tables are built on-demand using a new function which refactors much of the functionality of admin/index.php
          4. During each DB upgrade, the test tables are NOT upgraded.
          5. The discrepancy in version numbers following a DB upgrade will make unit tests disabled until the test DB is also upgraded (done on-demand like the install)
          6. An option remains to entirely drop and rebuild the test tables, once again using the new function described in 3.

          Regarding the cleaning up of data between each tests:

          1. Implement a lock so that unit tests can only be performed by one user concurrently
          2. In the test code, write a subclass of the DB object, overriding basic insert, update and delete functions so that a record is kept of the max ids of these tables the first time the function is called.
          3. In the test's tearDown() method, delete all records whose id is higher than that of the ids recorded in 2.

          Show
          Nicolas Connault added a comment - - edited After a long brainstorming session in Moodle HQ, here is the consensual decision: 1. DB tables with alternate prefix are required before Unit tests are available. 2. These tables are not built during install 3. The tables are built on-demand using a new function which refactors much of the functionality of admin/index.php 4. During each DB upgrade, the test tables are NOT upgraded. 5. The discrepancy in version numbers following a DB upgrade will make unit tests disabled until the test DB is also upgraded (done on-demand like the install) 6. An option remains to entirely drop and rebuild the test tables, once again using the new function described in 3. Regarding the cleaning up of data between each tests: 1. Implement a lock so that unit tests can only be performed by one user concurrently 2. In the test code, write a subclass of the DB object, overriding basic insert, update and delete functions so that a record is kept of the max ids of these tables the first time the function is called. 3. In the test's tearDown() method, delete all records whose id is higher than that of the ids recorded in 2.
          Hide
          Tim Hunt added a comment -

          Actually Nico, in the cleaning up list, in 2. we can override all update methods. It is easy to parse a list of table names out of any SQL statement because they are

          {in braces}

          .

          Show
          Tim Hunt added a comment - Actually Nico, in the cleaning up list, in 2. we can override all update methods. It is easy to parse a list of table names out of any SQL statement because they are {in braces} .
          Hide
          Nicolas Connault added a comment -

          In the cleaning up phase, an additional measure is needed to handle cases of fatal error during unit tests.

          1. Once at the beginning of the tests, save a flat text file with the highest PKs of all tables.
          2. Once all the tests are finished, delete the file
          3. If, when the tests are run next, the file is still there, it means the tests didn't complete properly the last time. The file is then used to truncate all records whose PKs are higher than the ones recorded. The file is kept.

          Show
          Nicolas Connault added a comment - In the cleaning up phase, an additional measure is needed to handle cases of fatal error during unit tests. 1. Once at the beginning of the tests, save a flat text file with the highest PKs of all tables. 2. Once all the tests are finished, delete the file 3. If, when the tests are run next, the file is still there, it means the tests didn't complete properly the last time. The file is then used to truncate all records whose PKs are higher than the ones recorded. The file is kept.
          Hide
          Petr Škoda added a comment -

          Please revert this DML prefix change
          +public function get_tables($prefix=null) {

          I tried very hard to prevent access to other tables with different prefixes in DML and this is totally against it. If you need to access other tables, please create a new database object with separate DB connection.

          Thanks

          Show
          Petr Škoda added a comment - Please revert this DML prefix change +public function get_tables($prefix=null) { I tried very hard to prevent access to other tables with different prefixes in DML and this is totally against it. If you need to access other tables, please create a new database object with separate DB connection. Thanks
          Hide
          Nicolas Connault added a comment -

          Almost finished all this, but hitting a problem with generation of courses: fix_course_sortorder(). It actually deletes/updates records that were NOT inserted during the unit test run. Any ideas on how to overcome this problem?

          Show
          Nicolas Connault added a comment - Almost finished all this, but hitting a problem with generation of courses: fix_course_sortorder(). It actually deletes/updates records that were NOT inserted during the unit test run. Any ideas on how to overcome this problem?
          Hide
          Penny Leach added a comment -

          extremely hacky way would be to examine the stack trace in your overridden DB object and ignore a call from fix_course_sortorder

          of course that does mean we can't test it.

          Show
          Penny Leach added a comment - extremely hacky way would be to examine the stack trace in your overridden DB object and ignore a call from fix_course_sortorder of course that does mean we can't test it.
          Hide
          Petr Škoda added a comment -

          This is a dead end, there is no way this would work reliably - we can not just switch $DB and hope that it will work fine because there are static and global caches and I am planning to add even more caching for performace reasons. There is a real danger that running unittest would somehow affect real moodle data this way.

          We will have to undo this and find some other solution I am afraid. This must be solved NOW, if you do not come with some suitable solution I will revert the code myself.

          Show
          Petr Škoda added a comment - This is a dead end, there is no way this would work reliably - we can not just switch $DB and hope that it will work fine because there are static and global caches and I am planning to add even more caching for performace reasons. There is a real danger that running unittest would somehow affect real moodle data this way. We will have to undo this and find some other solution I am afraid. This must be solved NOW, if you do not come with some suitable solution I will revert the code myself.
          Hide
          Petr Škoda added a comment -

          starting a refactoring that:

          • renames MoodleUnitTestCase to SeparateDBUnitTestCase - it must be clear that the real db emulation is not intended for normal users
          • review all unittests and make sure they use SeparateDBUnitTestCase really only when needed === revert many tests to simple UnitTestCase
          • remove creation of $CFG->unittestprefix, only manually created in config.php for developers
          • execute normal tests only if prefix not found
          • refactor db installation and remove test db upgrade; instead of upgrade add new cfg var which would indicate that main moodle or any plugin version changed - force reinstallation of all tables
          Show
          Petr Škoda added a comment - starting a refactoring that: renames MoodleUnitTestCase to SeparateDBUnitTestCase - it must be clear that the real db emulation is not intended for normal users review all unittests and make sure they use SeparateDBUnitTestCase really only when needed === revert many tests to simple UnitTestCase remove creation of $CFG->unittestprefix, only manually created in config.php for developers execute normal tests only if prefix not found refactor db installation and remove test db upgrade; instead of upgrade add new cfg var which would indicate that main moodle or any plugin version changed - force reinstallation of all tables
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Not sure if separating "safe" and "separate" unit tests is a good idea.

          I can imagine one function that being "safe" today (because it doesn't uses DB) can become "dangerous" tomorrow, because we change its internals (so should go to separate unit tests). I guess there is a 99% chance of forgetting to move that test from normal to separate, so there is a 99% chance of people executing it in the wrong (production) environment.

          I really think tests are a dangerous thing by definition and only should be allowed (all them) if:

          • The server isn't a production server ($CFG->productionserver=0).
          • The unittest prefix has been set (CFG->unittest) and isn't a substring of $DB->prefix (possible confilcts). But can be $DB->prefix (I think).
          • debugging mode is set to developer

          Summarizing, we must work to make unittest (all) useful for developers, allowing them to be re-executed safely and so on. But, at the same level, we must enforce the rule of not running them in production servers at all. Simply that. And, if that becomes true... then I cannot imagine any reason for having "normal" and "separate" unit tests. Difficult to maintain IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Not sure if separating "safe" and "separate" unit tests is a good idea. I can imagine one function that being "safe" today (because it doesn't uses DB) can become "dangerous" tomorrow, because we change its internals (so should go to separate unit tests). I guess there is a 99% chance of forgetting to move that test from normal to separate, so there is a 99% chance of people executing it in the wrong (production) environment. I really think tests are a dangerous thing by definition and only should be allowed (all them) if: The server isn't a production server ($CFG->productionserver=0). The unittest prefix has been set (CFG->unittest) and isn't a substring of $DB->prefix (possible confilcts). But can be $DB->prefix (I think). debugging mode is set to developer Summarizing, we must work to make unittest (all) useful for developers, allowing them to be re-executed safely and so on. But, at the same level, we must enforce the rule of not running them in production servers at all. Simply that. And, if that becomes true... then I cannot imagine any reason for having "normal" and "separate" unit tests. Difficult to maintain IMO. Ciao
          Hide
          Dan Poltawski added a comment -

          Agreed 100% with Eloy

          Show
          Dan Poltawski added a comment - Agreed 100% with Eloy
          Hide
          Petr Škoda added a comment -

          I agree with Eloy too
          and volunteering to do the coding...

          Show
          Petr Škoda added a comment - I agree with Eloy too and volunteering to do the coding...
          Hide
          Petr Škoda added a comment -

          alternative solution is to start using PHPUnit instead of unmaintained simpletest....

          Show
          Petr Škoda added a comment - alternative solution is to start using PHPUnit instead of unmaintained simpletest....
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Moving this to the DEV backlog for 2.1, assigning to Petr for further analysis/split. Note this cannot be performed in one unique sprint or so as far as it includes, at least:

          • Building the PHPUnit framework to be used from Moodle (interactive/cli)
          • Add the needed restrictions to avoid tests to be executed on production sites.
          • Convert gradually all the simpletests to phpunit ones
          • Support code coverage
          • Clean old tests.

          Surely this will end being some sort of META with a bunch of subtask to be assigned/completed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Moving this to the DEV backlog for 2.1, assigning to Petr for further analysis/split. Note this cannot be performed in one unique sprint or so as far as it includes, at least: Building the PHPUnit framework to be used from Moodle (interactive/cli) Add the needed restrictions to avoid tests to be executed on production sites. Convert gradually all the simpletests to phpunit ones Support code coverage Clean old tests. Surely this will end being some sort of META with a bunch of subtask to be assigned/completed. Ciao
          Hide
          Penny Leach added a comment -

          I don't know how you guys are doing scrum, but what you probably want for this initially is a spike: http://blog.agilebuddy.com/2009/11/what-is-a-spike-in-scrum.html which is usually a research/prototype task that is timeboxed and can be used to further split up the epic story into smaller stories that can then be placed in sprints.

          Show
          Penny Leach added a comment - I don't know how you guys are doing scrum, but what you probably want for this initially is a spike: http://blog.agilebuddy.com/2009/11/what-is-a-spike-in-scrum.html which is usually a research/prototype task that is timeboxed and can be used to further split up the epic story into smaller stories that can then be placed in sprints.
          Hide
          Petr Škoda added a comment -

          Penny: thanks for the link, we are not doing scrum yet, I wish there was someone already using scrum who would help us to plan these things a bit

          Show
          Petr Škoda added a comment - Penny: thanks for the link, we are not doing scrum yet, I wish there was someone already using scrum who would help us to plan these things a bit
          Hide
          Dan Poltawski added a comment -

          Petr, I think this old issue might be able to be closed with phpunit

          Show
          Dan Poltawski added a comment - Petr, I think this old issue might be able to be closed with phpunit
          Hide
          Petr Škoda added a comment -

          fixed, thanks

          Show
          Petr Škoda added a comment - fixed, thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved: