From 44121fbdc9b4005e054e9c8632058e7b9be1d0ff Mon Sep 17 00:00:00 2001 From: Rajesh Taneja Date: Wed, 11 Nov 2015 11:26:10 +0800 Subject: [PATCH 1/6] MDL-52060 phpunit: Keep track of initial reset sequence --- lib/testing/classes/util.php | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/lib/testing/classes/util.php b/lib/testing/classes/util.php index 4fdd3c74825..0864d3b6e87 100644 --- a/lib/testing/classes/util.php +++ b/lib/testing/classes/util.php @@ -59,6 +59,11 @@ abstract class testing_util { */ protected static $tablestructure = null; + /** + * @var array keep list of sequenceid used in a table. + */ + private static $tablesequences = array(); + /** * @var array original structure of all database tables */ @@ -78,6 +83,7 @@ abstract class testing_util { * @var int next sequence value for a single test cycle. */ protected static $sequencenextstartingid = null; + /** * Return the name of the JSON file containing the init filenames. * @@ -361,9 +367,10 @@ abstract class testing_util { // incorrect table match caused by _ continue; } + if (!is_null($info->auto_increment)) { $table = preg_replace('/^'.preg_quote($prefix, '/').'/', '', $table); - if ($info->auto_increment == 1) { + if (isset(self::$tablesequences[$table]) && ($info->auto_increment == self::$tablesequences[$table])) { $empties[$table] = $table; } } @@ -418,9 +425,14 @@ abstract class testing_util { * * @static * @param array $records The records to use to determine the starting value for the table. + * @param string $table table name. * @return int The value the sequence should be set to. */ - private static function get_next_sequence_starting_value($records) { + private static function get_next_sequence_starting_value($records, $table) { + if (isset(self::$tablesequences[$table])) { + return self::$tablesequences[$table]; + } + $id = self::$sequencenextstartingid; // If there are records, calculate the minimum id we can use. @@ -431,6 +443,9 @@ abstract class testing_util { } self::$sequencenextstartingid = $id + 1000; + + self::$tablesequences[$table] = $id; + return $id; } @@ -470,7 +485,7 @@ abstract class testing_util { $prefix = $DB->get_prefix(); foreach ($data as $table => $records) { if (isset($structure[$table]['id']) and $structure[$table]['id']->auto_increment) { - $nextid = self::get_next_sequence_starting_value($records); + $nextid = self::get_next_sequence_starting_value($records, $table); $queries[] = "ALTER SEQUENCE {$prefix}{$table}_id_seq RESTART WITH $nextid"; } } @@ -498,11 +513,10 @@ abstract class testing_util { foreach ($data as $table => $records) { if (isset($structure[$table]['id']) and $structure[$table]['id']->auto_increment) { if (isset($sequences[$table])) { - $nextid = self::get_next_sequence_starting_value($records); + $nextid = self::get_next_sequence_starting_value($records, $table); if ($sequences[$table] != $nextid) { $DB->change_database_structure("ALTER TABLE {$prefix}{$table} AUTO_INCREMENT = $nextid"); } - } else { // some problem exists, fallback to standard code $DB->get_manager()->reset_sequence($table); From 22d55b3950e033344eecc6ce0d1b71f5eec7c12c Mon Sep 17 00:00:00 2001 From: Rajesh Taneja Date: Wed, 11 Nov 2015 15:57:10 +0800 Subject: [PATCH 2/6] MDL-52060 phpunit: Check file only if needed --- lib/testing/classes/util.php | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/lib/testing/classes/util.php b/lib/testing/classes/util.php index 0864d3b6e87..1b08d6ff86a 100644 --- a/lib/testing/classes/util.php +++ b/lib/testing/classes/util.php @@ -269,17 +269,15 @@ abstract class testing_util { * @return array $table=>$records */ protected static function get_tabledata() { - global $CFG; - - $framework = self::get_framework(); - - $datafile = self::get_dataroot() . '/' . $framework . '/tabledata.ser'; - if (!file_exists($datafile)) { - // Not initialised yet. - return array(); - } - if (!isset(self::$tabledata)) { + $framework = self::get_framework(); + + $datafile = self::get_dataroot() . '/' . $framework . '/tabledata.ser'; + if (!file_exists($datafile)) { + // Not initialised yet. + return array(); + } + $data = file_get_contents($datafile); self::$tabledata = unserialize($data); } @@ -297,17 +295,15 @@ abstract class testing_util { * @return array $table=>$records */ public static function get_tablestructure() { - global $CFG; - - $framework = self::get_framework(); - - $structurefile = self::get_dataroot() . '/' . $framework . '/tablestructure.ser'; - if (!file_exists($structurefile)) { - // Not initialised yet. - return array(); - } - if (!isset(self::$tablestructure)) { + $framework = self::get_framework(); + + $structurefile = self::get_dataroot() . '/' . $framework . '/tablestructure.ser'; + if (!file_exists($structurefile)) { + // Not initialised yet. + return array(); + } + $data = file_get_contents($structurefile); self::$tablestructure = unserialize($data); } From d494306de7a930d51bec8022c60d937d3496d637 Mon Sep 17 00:00:00 2001 From: Mark Nielsen Date: Fri, 20 Nov 2015 13:53:22 -0800 Subject: [PATCH 3/6] MDL-52060 phpunit: Prevent querying database metadata --- lib/dml/moodle_database.php | 8 +++++--- lib/dml/tests/dml_test.php | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index 4540c7e1962..ea72527af59 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -577,6 +577,11 @@ abstract class moodle_database { protected function where_clause($table, array $conditions=null) { // We accept nulls in conditions $conditions = is_null($conditions) ? array() : $conditions; + + if (empty($conditions)) { + return array('', array()); + } + // Some checks performed under debugging only if (debugging()) { $columns = $this->get_columns($table); @@ -600,9 +605,6 @@ abstract class moodle_database { } $allowed_types = $this->allowed_param_types(); - if (empty($conditions)) { - return array('', array()); - } $where = array(); $params = array(); diff --git a/lib/dml/tests/dml_test.php b/lib/dml/tests/dml_test.php index 8b165652f0b..7e9dc550bf3 100644 --- a/lib/dml/tests/dml_test.php +++ b/lib/dml/tests/dml_test.php @@ -1432,9 +1432,9 @@ class core_dml_testcase extends database_driver_testcase { $this->assertSame('ddltablenotexist', $e->errorcode); } } - // And without params. + try { - $records = $DB->get_records('xxxx', array()); + $records = $DB->get_records('xxxx', array('id' => '1')); $this->fail('An Exception is missing, expected due to query against non-existing table'); } catch (moodle_exception $e) { $this->assertInstanceOf('dml_exception', $e); @@ -5347,7 +5347,7 @@ class core_dml_testcase extends database_driver_testcase { // The get_records() method generates 2 queries the first time is called // as it is fetching the table structure. - $whatever = $DB->get_records($tablename); + $whatever = $DB->get_records($tablename, array('id' => '1')); $this->assertEquals($initreads + 3, $DB->perf_get_reads()); $this->assertEquals($initwrites, $DB->perf_get_writes()); From e61a9638f115c166f6f65a714218098f99b9474e Mon Sep 17 00:00:00 2001 From: Mark Nielsen Date: Fri, 20 Nov 2015 13:55:27 -0800 Subject: [PATCH 4/6] MDL-52060 phpunit: Improve get_message_processors reset --- lib/phpunit/classes/util.php | 2 +- message/lib.php | 7 ++++++- message/upgrade.txt | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index 2321e4abafa..0da8931c898 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -207,7 +207,7 @@ class phpunit_util extends testing_util { reset_text_filters_cache(true); events_get_handlers('reset'); core_text::reset_caches(); - get_message_processors(false, true); + get_message_processors(false, true, true); filter_manager::reset_caches(); core_filetypes::reset_caches(); diff --git a/message/lib.php b/message/lib.php index e038f4e0686..52d2b7732dc 100644 --- a/message/lib.php +++ b/message/lib.php @@ -2571,14 +2571,19 @@ function message_mark_message_read($message, $timeread, $messageworkingempty=fal * * @param bool $ready only return ready-to-use processors * @param bool $reset Reset list of message processors (used in unit tests) + * @param bool $resetonly Just reset, then exit * @return mixed $processors array of objects containing information on message processors */ -function get_message_processors($ready = false, $reset = false) { +function get_message_processors($ready = false, $reset = false, $resetonly = false) { global $DB, $CFG; static $processors; if ($reset) { $processors = array(); + + if ($resetonly) { + return $processors; + } } if (empty($processors)) { diff --git a/message/upgrade.txt b/message/upgrade.txt index 91906a46407..24604cb05bb 100644 --- a/message/upgrade.txt +++ b/message/upgrade.txt @@ -1,6 +1,9 @@ This files describes API changes in /message/ messaging system, information provided here is intended especially for developers. +=== 3.1 === +* get_message_processors accepts an addition parameter for testing, which will just reset processor and exit. + === 2.9 === * Renderer method \core_message_renderer::manage_messagingoptions now accepts a user id parameter as well. From e487a51db3a929cc882abcf18a948dad8e2412ba Mon Sep 17 00:00:00 2001 From: Rajesh Taneja Date: Thu, 26 Nov 2015 15:31:56 +0800 Subject: [PATCH 5/6] MDL-52060 phpunit: Keep track of modified database for reset Keep list of modified tables, so they can be reset without any guess work --- lib/dml/moodle_database.php | 8 +++ lib/testing/classes/util.php | 109 ++++++++++++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index ea72527af59..89792c651e8 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -404,6 +404,14 @@ abstract class moodle_database { case SQL_QUERY_UPDATE: case SQL_QUERY_STRUCTURE: $this->writes++; + default: + if ((PHPUNIT_TEST) || (defined('BEHAT_TEST') && BEHAT_TEST) || + defined('BEHAT_SITE_RUNNING')) { + + // Set list of tables that are updated. + require_once(__DIR__.'/../testing/classes/util.php'); + testing_util::set_table_modified_by_sql($sql); + } } $this->print_debug($sql, $params); diff --git a/lib/testing/classes/util.php b/lib/testing/classes/util.php index 1b08d6ff86a..1017b1bbd67 100644 --- a/lib/testing/classes/util.php +++ b/lib/testing/classes/util.php @@ -64,6 +64,11 @@ abstract class testing_util { */ private static $tablesequences = array(); + /** + * @var array list of updated tables. + */ + public static $tableupdated = array(); + /** * @var array original structure of all database tables */ @@ -364,11 +369,9 @@ abstract class testing_util { continue; } - if (!is_null($info->auto_increment)) { + if (!is_null($info->auto_increment) && $info->rows == 0 && ($info->auto_increment == 1)) { $table = preg_replace('/^'.preg_quote($prefix, '/').'/', '', $table); - if (isset(self::$tablesequences[$table]) && ($info->auto_increment == self::$tablesequences[$table])) { - $empties[$table] = $table; - } + $empties[$table] = $table; } } $rs->close(); @@ -464,6 +467,8 @@ abstract class testing_util { return; } + $updatedtables = self::$tableupdated; + // If all starting Id's are the same, it's difficult to detect coding and testing // errors that use the incorrect id in tests. The classic case is cmid vs instance id. // To reduce the chance of the coding error, we start sequences at different values where possible. @@ -480,6 +485,10 @@ abstract class testing_util { $queries = array(); $prefix = $DB->get_prefix(); foreach ($data as $table => $records) { + // If table is not modified then no need to do anything. + if (!isset($updatedtables[$table])) { + continue; + } if (isset($structure[$table]['id']) and $structure[$table]['id']->auto_increment) { $nextid = self::get_next_sequence_starting_value($records, $table); $queries[] = "ALTER SEQUENCE {$prefix}{$table}_id_seq RESTART WITH $nextid"; @@ -490,6 +499,7 @@ abstract class testing_util { } } else if ($dbfamily === 'mysql') { + $queries = array(); $sequences = array(); $prefix = $DB->get_prefix(); $rs = $DB->get_recordset_sql("SHOW TABLE STATUS LIKE ?", array($prefix.'%')); @@ -507,11 +517,15 @@ abstract class testing_util { $rs->close(); $prefix = $DB->get_prefix(); foreach ($data as $table => $records) { + // If table is not modified then no need to do anything. + if (!isset($updatedtables[$table])) { + continue; + } if (isset($structure[$table]['id']) and $structure[$table]['id']->auto_increment) { if (isset($sequences[$table])) { $nextid = self::get_next_sequence_starting_value($records, $table); if ($sequences[$table] != $nextid) { - $DB->change_database_structure("ALTER TABLE {$prefix}{$table} AUTO_INCREMENT = $nextid"); + $queries[] = "ALTER TABLE {$prefix}{$table} AUTO_INCREMENT = $nextid"; } } else { // some problem exists, fallback to standard code @@ -519,6 +533,9 @@ abstract class testing_util { } } } + if ($queries) { + $DB->change_database_structure(implode(';', $queries)); + } } else if ($dbfamily === 'oracle') { $sequences = self::get_sequencenames(); @@ -536,6 +553,10 @@ abstract class testing_util { $rs->close(); foreach ($data as $table => $records) { + // If table is not modified then no need to do anything. + if (!isset($updatedtables[$table])) { + continue; + } if (isset($structure[$table]['id']) and $structure[$table]['id']->auto_increment) { $lastrecord = end($records); if ($lastrecord) { @@ -559,11 +580,12 @@ abstract class testing_util { } else { // note: does mssql support any kind of faster reset? // This also implies mssql will not use unique sequence values. - if (is_null($empties)) { + if (is_null($empties) and (empty($updatedtables))) { $empties = self::guess_unmodified_empty_tables(); } foreach ($data as $table => $records) { - if (isset($empties[$table])) { + // If table is not modified then no need to do anything. + if (isset($empties[$table]) or (!isset($updatedtables[$table]))) { continue; } if (isset($structure[$table]['id']) and $structure[$table]['id']->auto_increment) { @@ -596,7 +618,26 @@ abstract class testing_util { return false; } - $empties = self::guess_unmodified_empty_tables(); + $empties = array(); + // Use local copy of self::$tableupdated, as list gets updated in for loop. + $updatedtables = self::$tableupdated; + + // If empty tablesequences list then it's the very first run. + if (empty(self::$tablesequences) && (($DB->get_dbfamily() != 'mysql') && ($DB->get_dbfamily() != 'postgres'))) { + // Only Mysql and Postgres support random sequence, so don't guess, just reset everything on very first run. + $empties = self::guess_unmodified_empty_tables(); + } + + // Check if any table has been modified by behat selenium process. + if (defined('BEHAT_SITE_RUNNING')) { + // Crazy way to reset :(. + $tablesupdatedfile = self::get_tables_updated_by_scenario_list_path(); + if ($tablesupdated = @json_decode(file_get_contents($tablesupdatedfile), true)) { + self::$tableupdated = array_merge(self::$tableupdated, $tablesupdated); + unlink($tablesupdatedfile); + } + $updatedtables = self::$tableupdated; + } $borkedmysql = false; if ($DB->get_dbfamily() === 'mysql') { @@ -637,6 +678,12 @@ abstract class testing_util { } foreach ($data as $table => $records) { + // If table is not modified then no need to do anything. + // $updatedtables tables is set after the first run, so check before checking for specific table update. + if (!empty($updatedtables) && !isset($updatedtables[$table])) { + continue; + } + if ($borkedmysql) { if (empty($records) and isset($empties[$table])) { continue; @@ -709,6 +756,8 @@ abstract class testing_util { } } + self::reset_updated_table_list(); + return true; } @@ -843,6 +892,50 @@ abstract class testing_util { return $hash; } + /** + * Set state of modified tables. + * + * @param string $sql sql which is updating the table. + */ + public static function set_table_modified_by_sql($sql) { + global $DB; + + $prefix = $DB->get_prefix(); + + preg_match('/( ' . $prefix . '\w*)(.*)/', $sql, $matches); + // Ignore random sql for testing like "XXUPDATE SET XSSD". + if (!empty($matches[1])) { + $table = trim($matches[1]); + $table = preg_replace('/^' . preg_quote($prefix, '/') . '/', '', $table); + self::$tableupdated[$table] = true; + + if (defined('BEHAT_SITE_RUNNING')) { + $tablesupdatedfile = self::get_tables_updated_by_scenario_list_path(); + if ($tablesupdated = @json_decode(file_get_contents($tablesupdatedfile), true)) { + $tablesupdated[$table] = true; + } else { + $tablesupdated[$table] = true; + } + @file_put_contents($tablesupdatedfile, json_encode($tablesupdated, JSON_PRETTY_PRINT)); + } + } + } + + /** + * Reset updated table list. This should be done after every reset. + */ + public static function reset_updated_table_list() { + self::$tableupdated = array(); + } + + /** + * Returns the path to the file which holds list of tables updated in scenario. + * @return string + */ + protected final static function get_tables_updated_by_scenario_list_path() { + return self::get_dataroot() . '/tablesupdatedbyscenario.txt'; + } + /** * Drop the whole test database * @static From 46c18e0dd257f7df4b136601932aa17085f74e2c Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Tue, 22 Dec 2015 11:42:35 +0800 Subject: [PATCH 6/6] MDL-52060 phpunit: Add tests for test_set_table_modified_by_sql --- lib/phpunit/tests/util_test.php | 105 ++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 lib/phpunit/tests/util_test.php diff --git a/lib/phpunit/tests/util_test.php b/lib/phpunit/tests/util_test.php new file mode 100644 index 00000000000..e59d95b7a0c --- /dev/null +++ b/lib/phpunit/tests/util_test.php @@ -0,0 +1,105 @@ +. + +/** + * PHPUnit Util tests + * + * @package core + * @category phpunit + * @copyright 2015 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +defined('MOODLE_INTERNAL') || die(); + +/** + * Test util extra features. + * + * @package core + * @category phpunit + * @copyright 2015 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class core_phpunit_util_testcase extends advanced_testcase { + /** + * @dataProvider set_table_modified_by_sql_provider + */ + public function test_set_table_modified_by_sql($sql, $expectations) { + phpunit_util::reset_updated_table_list(); + phpunit_util::set_table_modified_by_sql($sql); + foreach ($expectations as $table => $present) { + $this->assertEquals($present, !empty(phpunit_util::$tableupdated[$table])); + } + } + + public function set_table_modified_by_sql_provider() { + global $DB; + $prefix = $DB->get_prefix(); + + return array( + 'Basic update' => array( + 'sql' => "UPDATE {$prefix}user SET username = username || '_test'", + 'expectations' => array( + 'user' => true, + 'course' => false, + ), + ), + 'Basic update with a fieldname sharing the same prefix' => array( + 'sql' => "UPDATE {$prefix}user SET {$prefix}username = username || '_test'", + 'expectations' => array( + 'user' => true, + 'course' => false, + ), + ), + 'Basic update with a table which contains the prefix' => array( + 'sql' => "UPDATE {$prefix}user{$prefix} SET username = username || '_test'", + 'expectations' => array( + "user{$prefix}" => true, + 'course' => false, + ), + ), + 'Update table with a numeric name' => array( + 'sql' => "UPDATE {$prefix}example42 SET username = username || '_test'", + 'expectations' => array( + 'example42' => true, + 'user' => false, + 'course' => false, + ), + ), + 'Drop basic table' => array( + 'sql' => "DROP TABLE {$prefix}user", + 'expectations' => array( + 'user' => true, + 'course' => false, + ), + ), + 'Drop table with a numeric name' => array( + 'sql' => "DROP TABLE {$prefix}example42", + 'expectations' => array( + 'example42' => true, + 'user' => false, + 'course' => false, + ), + ), + 'Insert in table' => array( + 'sql' => "INSERT INTO {$prefix}user (username,password) VALUES ('moodle', 'test')", + 'expectations' => array( + 'user' => true, + 'course' => false, + ), + ), + ); + } +}