From a946a9aac6ea3d1300fe74f935192f1957f6a84e Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Sun, 10 Nov 2019 20:53:25 +0800 Subject: [PATCH 1/5] MDL-67353 course: Fix broken unit test --- course/classes/customfield/course_handler.php | 11 ++++++++++ customfield/tests/api_test.php | 22 ++++++++++--------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/course/classes/customfield/course_handler.php b/course/classes/customfield/course_handler.php index d7badc85bf7..5deeb4737ff 100644 --- a/course/classes/customfield/course_handler.php +++ b/course/classes/customfield/course_handler.php @@ -68,6 +68,17 @@ class course_handler extends \core_customfield\handler { return self::$singleton; } + /** + * Run reset code after unit tests to reset the singleton usage. + */ + public static function reset_after_test(): void { + if (!PHPUNIT_TEST) { + throw new \coding_exception('This feature is only intended for use in unit tests'); + } + + static::$singleton = null; + } + /** * The current user can configure custom fields on this component. * diff --git a/customfield/tests/api_test.php b/customfield/tests/api_test.php index 2aa96e1883b..4f8c3c26dbc 100644 --- a/customfield/tests/api_test.php +++ b/customfield/tests/api_test.php @@ -40,16 +40,8 @@ class core_customfield_api_testcase extends advanced_testcase { /** * This method is called after the last test of this test class is run. */ - public static function tearDownAfterClass() { - $handler = core_course\customfield\course_handler::create(); - $handler->delete_all(); - } - - /** - * Tests set up. - */ - public function setUp() { - $this->resetAfterTest(); + public function tearDown() { + core_course\customfield\course_handler::reset_after_test(); } /** @@ -80,6 +72,8 @@ class core_customfield_api_testcase extends advanced_testcase { * in the interface using drag-drop. */ public function test_move_category() { + $this->resetAfterTest(); + // Create the categories. $params = ['component' => 'core_course', 'area' => 'course', 'itemid' => 0]; $id0 = $this->get_generator()->create_category($params)->get('id'); @@ -129,6 +123,8 @@ class core_customfield_api_testcase extends advanced_testcase { * Tests for \core_customfield\api::get_categories_with_fields() behaviour. */ public function test_get_categories_with_fields() { + $this->resetAfterTest(); + // Create the categories. $options = [ 'component' => 'core_course', @@ -163,6 +159,8 @@ class core_customfield_api_testcase extends advanced_testcase { * Test for functions api::save_category() and rename_category) */ public function test_save_category() { + $this->resetAfterTest(); + $params = ['component' => 'core_course', 'area' => 'course', 'itemid' => 0, 'name' => 'Cat1', 'contextid' => context_system::instance()->id]; $c1 = category_controller::create(0, (object)$params); @@ -196,6 +194,8 @@ class core_customfield_api_testcase extends advanced_testcase { * Test for function handler::create_category */ public function test_create_category() { + $this->resetAfterTest(); + $handler = \core_course\customfield\course_handler::create(); $c1id = $handler->create_category(); $c1 = $handler->get_categories_with_fields()[$c1id]; @@ -218,6 +218,8 @@ class core_customfield_api_testcase extends advanced_testcase { * Tests for \core_customfield\api::delete_category() behaviour. */ public function test_delete_category_with_fields() { + $this->resetAfterTest(); + global $DB; // Create two categories with fields and data. $options = [ From b2e08b7caa0ceec113ced3353dea6bdd8f468779 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 11 Nov 2019 07:08:28 +0800 Subject: [PATCH 2/5] MDL-67353 course: Fix more broken unit tests --- customfield/tests/api_test.php | 10 +- .../tests/category_controller_test.php | 32 ++-- customfield/tests/data_controller_test.php | 26 ++- customfield/tests/field_controller_test.php | 32 ++-- customfield/tests/generator_test.php | 10 +- customfield/tests/privacy_test.php | 169 +++++++++++------- 6 files changed, 164 insertions(+), 115 deletions(-) diff --git a/customfield/tests/api_test.php b/customfield/tests/api_test.php index 4f8c3c26dbc..434eb355bae 100644 --- a/customfield/tests/api_test.php +++ b/customfield/tests/api_test.php @@ -38,17 +38,19 @@ use \core_customfield\category_controller; class core_customfield_api_testcase extends advanced_testcase { /** - * This method is called after the last test of this test class is run. + * Tear down to reset the singleton after each test. */ public function tearDown() { core_course\customfield\course_handler::reset_after_test(); + parent::tearDown(); } /** - * Get generator + * Get generator. + * * @return core_customfield_generator */ - protected function get_generator() : core_customfield_generator { + protected function get_generator(): core_customfield_generator { return $this->getDataGenerator()->get_plugin_generator('core_customfield'); } @@ -57,7 +59,7 @@ class core_customfield_api_testcase extends advanced_testcase { * * @param array $expected * @param array $array array of objects with "get($property)" method - * @param sring $propertyname + * @param string $propertyname */ protected function assert_property_in_array($expected, $array, $propertyname) { $this->assertEquals($expected, array_values(array_map(function($a) use ($propertyname) { diff --git a/customfield/tests/category_controller_test.php b/customfield/tests/category_controller_test.php index 56aa2d0d5ce..210a9669ba3 100644 --- a/customfield/tests/category_controller_test.php +++ b/customfield/tests/category_controller_test.php @@ -37,29 +37,28 @@ use \core_customfield\field_controller; class core_customfield_category_controller_testcase extends advanced_testcase { /** - * This method is called after the last test of this test class is run. + * Tear down to reset the singleton after each test. */ - public static function tearDownAfterClass() { - $handler = core_course\customfield\course_handler::create(); - $handler->delete_all(); + public function tearDown() { + core_course\customfield\course_handler::reset_after_test(); + parent::tearDown(); } /** - * Tests set up. - */ - public function setUp() { - $this->resetAfterTest(); - } - - /** - * Get generator + * Get generator. + * * @return core_customfield_generator */ - protected function get_generator() : core_customfield_generator { + protected function get_generator(): core_customfield_generator { return $this->getDataGenerator()->get_plugin_generator('core_customfield'); } + /** + * Test for the field_controller::__construct function. + */ public function test_constructor() { + $this->resetAfterTest(); + $c = category_controller::create(0, (object)['component' => 'core_course', 'area' => 'course', 'itemid' => 0]); $handler = $c->get_handler(); $this->assertTrue($c instanceof category_controller); @@ -83,6 +82,8 @@ class core_customfield_category_controller_testcase extends advanced_testcase { */ public function test_constructor_errors() { global $DB; + $this->resetAfterTest(); + $cat = $this->get_generator()->create_category(); $catrecord = $cat->to_record(); @@ -181,6 +182,7 @@ class core_customfield_category_controller_testcase extends advanced_testcase { * \core_customfield\category_controller::get() */ public function test_create_category() { + $this->resetAfterTest(); // Create the category. $lpg = $this->get_generator(); @@ -209,6 +211,8 @@ class core_customfield_category_controller_testcase extends advanced_testcase { * Tests for \core_customfield\category_controller::set() behaviour. */ public function test_rename_category() { + $this->resetAfterTest(); + // Create the category. $params = ['component' => 'core_course', 'area' => 'course', 'itemid' => 0, 'name' => 'Cat1', 'contextid' => context_system::instance()->id]; @@ -232,6 +236,8 @@ class core_customfield_category_controller_testcase extends advanced_testcase { * Tests for \core_customfield\category_controller::delete() behaviour. */ public function test_delete_category() { + $this->resetAfterTest(); + // Create the category. $lpg = $this->get_generator(); $category0 = $lpg->create_category(); diff --git a/customfield/tests/data_controller_test.php b/customfield/tests/data_controller_test.php index 7cf0b7ebac0..ab5b9da3806 100644 --- a/customfield/tests/data_controller_test.php +++ b/customfield/tests/data_controller_test.php @@ -35,25 +35,19 @@ use core_customfield\data_controller; class core_customfield_data_controller_testcase extends advanced_testcase { /** - * This method is called after the last test of this test class is run. + * Tear down to reset the singleton after each test. */ - public static function tearDownAfterClass() { - $handler = core_course\customfield\course_handler::create(); - $handler->delete_all(); + public function tearDown() { + core_course\customfield\course_handler::reset_after_test(); + parent::tearDown(); } /** - * Tests set up. - */ - public function setUp() { - $this->resetAfterTest(); - } - - /** - * Get generator + * Get generator. + * * @return core_customfield_generator */ - protected function get_generator() : core_customfield_generator { + protected function get_generator(): core_customfield_generator { return $this->getDataGenerator()->get_plugin_generator('core_customfield'); } @@ -62,6 +56,8 @@ class core_customfield_data_controller_testcase extends advanced_testcase { */ public function test_constructor() { global $DB; + $this->resetAfterTest(); + // Create a course, fields category and fields. $course = $this->getDataGenerator()->create_course(); $category0 = $this->get_generator()->create_category(['name' => 'aaaa']); @@ -130,6 +126,8 @@ class core_customfield_data_controller_testcase extends advanced_testcase { */ public function test_constructor_errors() { global $DB; + $this->resetAfterTest(); + // Create a category, field and data. $category = $this->get_generator()->create_category(); $field = $this->get_generator()->create_field(['categoryid' => $category->get('id')]); @@ -188,4 +186,4 @@ class core_customfield_data_controller_testcase extends advanced_testcase { $this->assertEquals(moodle_exception::class, get_class($e)); } } -} \ No newline at end of file +} diff --git a/customfield/tests/field_controller_test.php b/customfield/tests/field_controller_test.php index e490ef1b07a..ff26572fe05 100644 --- a/customfield/tests/field_controller_test.php +++ b/customfield/tests/field_controller_test.php @@ -39,25 +39,19 @@ use \core_customfield\field_controller; class core_customfield_field_controller_testcase extends advanced_testcase { /** - * This method is called after the last test of this test class is run. + * Tear down to reset the singleton after each test. */ - public static function tearDownAfterClass() { - $handler = core_course\customfield\course_handler::create(); - $handler->delete_all(); + public function tearDown() { + core_course\customfield\course_handler::reset_after_test(); + parent::tearDown(); } /** - * Tests set up. - */ - public function setUp() { - $this->resetAfterTest(); - } - - /** - * Get generator + * Get generator. + * * @return core_customfield_generator */ - protected function get_generator() : core_customfield_generator { + protected function get_generator(): core_customfield_generator { return $this->getDataGenerator()->get_plugin_generator('core_customfield'); } @@ -66,6 +60,8 @@ class core_customfield_field_controller_testcase extends advanced_testcase { */ public function test_constructor() { global $DB; + $this->resetAfterTest(); + // Create the category. $category0 = $this->get_generator()->create_category(); @@ -111,6 +107,8 @@ class core_customfield_field_controller_testcase extends advanced_testcase { */ public function test_constructor_errors() { global $DB; + $this->resetAfterTest(); + // Create a category and a field. $category = $this->get_generator()->create_category(); $field = $this->get_generator()->create_field(['categoryid' => $category->get('id')]); @@ -183,6 +181,8 @@ class core_customfield_field_controller_testcase extends advanced_testcase { */ public function test_create_field() { global $DB; + $this->resetAfterTest(); + $lpg = $this->get_generator(); $category = $lpg->create_category(); $fields = $DB->get_records(\core_customfield\field::TABLE, ['categoryid' => $category->get('id')]); @@ -211,6 +211,8 @@ class core_customfield_field_controller_testcase extends advanced_testcase { */ public function test_delete_field() { global $DB; + $this->resetAfterTest(); + $lpg = $this->get_generator(); $category = $lpg->create_category(); $fields = $DB->get_records(\core_customfield\field::TABLE, ['categoryid' => $category->get('id')]); @@ -237,6 +239,8 @@ class core_customfield_field_controller_testcase extends advanced_testcase { * Tests for \core_customfield\field_controller::get_configdata_property() behaviour. */ public function test_get_configdata_property() { + $this->resetAfterTest(); + $lpg = $this->get_generator(); $category = $lpg->create_category(); $configdata = ['a' => 'b', 'c' => ['d', 'e']]; @@ -251,4 +255,4 @@ class core_customfield_field_controller_testcase extends advanced_testcase { $this->assertEquals(['d', 'e'], $field->get_configdata_property('c')); $this->assertEquals(null, $field->get_configdata_property('x')); } -} \ No newline at end of file +} diff --git a/customfield/tests/generator_test.php b/customfield/tests/generator_test.php index 059c868a8dd..38d8ebb9f84 100644 --- a/customfield/tests/generator_test.php +++ b/customfield/tests/generator_test.php @@ -36,18 +36,18 @@ defined('MOODLE_INTERNAL') || die(); class core_customfield_generator_testcase extends advanced_testcase { /** - * This method is called after the last test of this test class is run. + * Tear down to reset the singleton after each test. */ - public static function tearDownAfterClass() { - $handler = core_course\customfield\course_handler::create(); - $handler->delete_all(); + public function tearDown() { + core_course\customfield\course_handler::reset_after_test(); + parent::tearDown(); } /** * Get generator * @return core_customfield_generator */ - protected function get_generator() : core_customfield_generator { + protected function get_generator(): core_customfield_generator { return $this->getDataGenerator()->get_plugin_generator('core_customfield'); } diff --git a/customfield/tests/privacy_test.php b/customfield/tests/privacy_test.php index 69ea102f65d..bdb5d374b72 100644 --- a/customfield/tests/privacy_test.php +++ b/customfield/tests/privacy_test.php @@ -38,71 +38,67 @@ use core_customfield\privacy\provider; */ class core_customfield_privacy_testcase extends provider_testcase { - /** @var stdClass[] */ - private $courses = []; - /** @var \core_customfield\category_controller[] */ - private $cfcats = []; - /** @var \core_customfield\field_controller[] */ - private $cffields = []; - /** - * This method is called after the last test of this test class is run. + * Tear down to reset the singleton after each test. */ - public static function tearDownAfterClass() { - $handler = core_course\customfield\course_handler::create(); - $handler->delete_all(); + public function tearDown() { + core_course\customfield\course_handler::reset_after_test(); + parent::tearDown(); } /** - * Set up + * Generate data. + * + * @return array */ - public function setUp() { + protected function generate_test_data(): array { $this->resetAfterTest(); - $this->cfcats[1] = $this->get_generator()->create_category(); - $this->cfcats[2] = $this->get_generator()->create_category(); - $this->cffields[11] = $this->get_generator()->create_field( - ['categoryid' => $this->cfcats[1]->get('id'), 'type' => 'checkbox']); - $this->cffields[12] = $this->get_generator()->create_field( - ['categoryid' => $this->cfcats[1]->get('id'), 'type' => 'date']); - $this->cffields[13] = $this->get_generator()->create_field( - ['categoryid' => $this->cfcats[1]->get('id'), + $generator = $this->getDataGenerator()->get_plugin_generator('core_customfield'); + $cfcats[1] = $generator->create_category(); + $cfcats[2] = $generator->create_category(); + $cffields[11] = $generator->create_field( + ['categoryid' => $cfcats[1]->get('id'), 'type' => 'checkbox']); + $cffields[12] = $generator->create_field( + ['categoryid' => $cfcats[1]->get('id'), 'type' => 'date']); + $cffields[13] = $generator->create_field( + ['categoryid' => $cfcats[1]->get('id'), 'type' => 'select', 'configdata' => ['options' => "a\nb\nc"]]); - $this->cffields[14] = $this->get_generator()->create_field( - ['categoryid' => $this->cfcats[1]->get('id'), 'type' => 'text']); - $this->cffields[15] = $this->get_generator()->create_field( - ['categoryid' => $this->cfcats[1]->get('id'), 'type' => 'textarea']); - $this->cffields[21] = $this->get_generator()->create_field( - ['categoryid' => $this->cfcats[2]->get('id')]); - $this->cffields[22] = $this->get_generator()->create_field( - ['categoryid' => $this->cfcats[2]->get('id')]); + $cffields[14] = $generator->create_field( + ['categoryid' => $cfcats[1]->get('id'), 'type' => 'text']); + $cffields[15] = $generator->create_field( + ['categoryid' => $cfcats[1]->get('id'), 'type' => 'textarea']); + $cffields[21] = $generator->create_field( + ['categoryid' => $cfcats[2]->get('id')]); + $cffields[22] = $generator->create_field( + ['categoryid' => $cfcats[2]->get('id')]); - $this->courses[1] = $this->getDataGenerator()->create_course(); - $this->courses[2] = $this->getDataGenerator()->create_course(); - $this->courses[3] = $this->getDataGenerator()->create_course(); + $courses[1] = $this->getDataGenerator()->create_course(); + $courses[2] = $this->getDataGenerator()->create_course(); + $courses[3] = $this->getDataGenerator()->create_course(); - $this->get_generator()->add_instance_data($this->cffields[11], $this->courses[1]->id, 1); - $this->get_generator()->add_instance_data($this->cffields[12], $this->courses[1]->id, 1546300800); - $this->get_generator()->add_instance_data($this->cffields[13], $this->courses[1]->id, 2); - $this->get_generator()->add_instance_data($this->cffields[14], $this->courses[1]->id, 'Hello1'); - $this->get_generator()->add_instance_data($this->cffields[15], $this->courses[1]->id, + $generator->add_instance_data($cffields[11], $courses[1]->id, 1); + $generator->add_instance_data($cffields[12], $courses[1]->id, 1546300800); + $generator->add_instance_data($cffields[13], $courses[1]->id, 2); + $generator->add_instance_data($cffields[14], $courses[1]->id, 'Hello1'); + $generator->add_instance_data($cffields[15], $courses[1]->id, ['text' => '

Hi there

', 'format' => FORMAT_HTML]); - $this->get_generator()->add_instance_data($this->cffields[21], $this->courses[1]->id, 'hihi1'); + $generator->add_instance_data($cffields[21], $courses[1]->id, 'hihi1'); - $this->get_generator()->add_instance_data($this->cffields[14], $this->courses[2]->id, 'Hello2'); + $generator->add_instance_data($cffields[14], $courses[2]->id, 'Hello2'); - $this->get_generator()->add_instance_data($this->cffields[21], $this->courses[2]->id, 'hihi2'); + $generator->add_instance_data($cffields[21], $courses[2]->id, 'hihi2'); - $this->setUser($this->getDataGenerator()->create_user()); - } + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); - /** - * Get generator - * @return core_customfield_generator - */ - protected function get_generator() : core_customfield_generator { - return $this->getDataGenerator()->get_plugin_generator('core_customfield'); + return [ + 'user' => $user, + 'cfcats' => $cfcats, + 'cffields' => $cffields, + 'courses' => $courses, + ]; } /** @@ -119,11 +115,17 @@ class core_customfield_privacy_testcase extends provider_testcase { */ public function test_get_customfields_data_contexts() { global $DB; - list($sql, $params) = $DB->get_in_or_equal([$this->courses[1]->id, $this->courses[2]->id], SQL_PARAMS_NAMED); + [ + 'cffields' => $cffields, + 'cfcats' => $cfcats, + 'courses' => $courses, + ] = $this->generate_test_data(); + + list($sql, $params) = $DB->get_in_or_equal([$courses[1]->id, $courses[2]->id], SQL_PARAMS_NAMED); $r = provider::get_customfields_data_contexts('core_course', 'course', '=0', $sql, $params); - $this->assertEquals([context_course::instance($this->courses[1]->id)->id, - context_course::instance($this->courses[2]->id)->id], + $this->assertEquals([context_course::instance($courses[1]->id)->id, + context_course::instance($courses[2]->id)->id], $r->get_contextids(), '', 0, 10, true); } @@ -131,6 +133,8 @@ class core_customfield_privacy_testcase extends provider_testcase { * Test for provider::get_customfields_configuration_contexts() */ public function test_get_customfields_configuration_contexts() { + $this->generate_test_data(); + $r = provider::get_customfields_configuration_contexts('core_course', 'course'); $this->assertEquals([context_system::instance()->id], $r->get_contextids()); } @@ -140,13 +144,20 @@ class core_customfield_privacy_testcase extends provider_testcase { */ public function test_export_customfields_data() { global $USER, $DB; + $this->resetAfterTest(); + [ + 'cffields' => $cffields, + 'cfcats' => $cfcats, + 'courses' => $courses, + ] = $this->generate_test_data(); + // Hack one of the fields so it has an invalid field type. - $invalidfieldid = $this->cffields[21]->get('id'); + $invalidfieldid = $cffields[21]->get('id'); $DB->update_record('customfield_field', ['id' => $invalidfieldid, 'type' => 'invalid']); - $context = context_course::instance($this->courses[1]->id); + $context = context_course::instance($courses[1]->id); $contextlist = new approved_contextlist($USER, 'core_customfield', [$context->id]); - provider::export_customfields_data($contextlist, 'core_course', 'course', '=0', '=:i', ['i' => $this->courses[1]->id]); + provider::export_customfields_data($contextlist, 'core_course', 'course', '=0', '=:i', ['i' => $courses[1]->id]); /** @var core_privacy\tests\request\content_writer $writer */ $writer = writer::with_context($context); @@ -155,7 +166,7 @@ class core_customfield_privacy_testcase extends provider_testcase { $invaldfieldischecked = false; foreach ($DB->get_records('customfield_data', []) as $dbrecord) { $data = $writer->get_data(['Custom fields data', $dbrecord->id]); - if ($dbrecord->instanceid == $this->courses[1]->id) { + if ($dbrecord->instanceid == $courses[1]->id) { $this->assertEquals($dbrecord->fieldid, $data->fieldid); $this->assertNotEmpty($data->fieldtype); $this->assertNotEmpty($data->fieldshortname); @@ -175,10 +186,17 @@ class core_customfield_privacy_testcase extends provider_testcase { */ public function test_delete_customfields_data() { global $USER, $DB; - $approvedcontexts = new approved_contextlist($USER, 'core_course', [context_course::instance($this->courses[1]->id)->id]); + $this->resetAfterTest(); + [ + 'cffields' => $cffields, + 'cfcats' => $cfcats, + 'courses' => $courses, + ] = $this->generate_test_data(); + + $approvedcontexts = new approved_contextlist($USER, 'core_course', [context_course::instance($courses[1]->id)->id]); provider::delete_customfields_data($approvedcontexts, 'core_course', 'course'); - $this->assertEmpty($DB->get_records('customfield_data', ['instanceid' => $this->courses[1]->id])); - $this->assertNotEmpty($DB->get_records('customfield_data', ['instanceid' => $this->courses[2]->id])); + $this->assertEmpty($DB->get_records('customfield_data', ['instanceid' => $courses[1]->id])); + $this->assertNotEmpty($DB->get_records('customfield_data', ['instanceid' => $courses[2]->id])); } /** @@ -186,9 +204,16 @@ class core_customfield_privacy_testcase extends provider_testcase { */ public function test_delete_customfields_configuration() { global $USER, $DB; + $this->resetAfterTest(); + [ + 'cffields' => $cffields, + 'cfcats' => $cfcats, + 'courses' => $courses, + ] = $this->generate_test_data(); + // Remember the list of fields in the category 2 before we delete it. - $catid1 = $this->cfcats[1]->get('id'); - $catid2 = $this->cfcats[2]->get('id'); + $catid1 = $cfcats[1]->get('id'); + $catid2 = $cfcats[2]->get('id'); $fids2 = $DB->get_fieldset_select('customfield_field', 'id', 'categoryid=?', [$catid2]); $this->assertNotEmpty($fids2); list($fsql, $fparams) = $DB->get_in_or_equal($fids2, SQL_PARAMS_NAMED); @@ -216,9 +241,16 @@ class core_customfield_privacy_testcase extends provider_testcase { */ public function test_delete_customfields_configuration_for_context() { global $USER, $DB; + $this->resetAfterTest(); + [ + 'cffields' => $cffields, + 'cfcats' => $cfcats, + 'courses' => $courses, + ] = $this->generate_test_data(); + // Remember the list of fields in the category 2 before we delete it. - $catid1 = $this->cfcats[1]->get('id'); - $catid2 = $this->cfcats[2]->get('id'); + $catid1 = $cfcats[1]->get('id'); + $catid2 = $cfcats[2]->get('id'); $fids2 = $DB->get_fieldset_select('customfield_field', 'id', 'categoryid=?', [$catid2]); $this->assertNotEmpty($fids2); list($fsql, $fparams) = $DB->get_in_or_equal($fids2, SQL_PARAMS_NAMED); @@ -246,12 +278,19 @@ class core_customfield_privacy_testcase extends provider_testcase { */ public function test_delete_customfields_data_for_context() { global $DB; + $this->resetAfterTest(); + [ + 'cffields' => $cffields, + 'cfcats' => $cfcats, + 'courses' => $courses, + ] = $this->generate_test_data(); + provider::delete_customfields_data_for_context('core_course', 'course', - context_course::instance($this->courses[1]->id)); + context_course::instance($courses[1]->id)); $fids2 = $DB->get_fieldset_select('customfield_field', 'id', '1=1', []); list($fsql, $fparams) = $DB->get_in_or_equal($fids2, SQL_PARAMS_NAMED); - $fparams['course1'] = $this->courses[1]->id; - $fparams['course2'] = $this->courses[2]->id; + $fparams['course1'] = $courses[1]->id; + $fparams['course2'] = $courses[2]->id; $this->assertEmpty($DB->get_records_select('customfield_data', 'instanceid = :course1 AND fieldid ' . $fsql, $fparams)); $this->assertNotEmpty($DB->get_records_select('customfield_data', 'instanceid = :course2 AND fieldid ' . $fsql, $fparams)); } From 89a4d27f3496b1ef7f12d117dfe4043c99950d9b Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sun, 12 Jan 2020 13:20:33 +0100 Subject: [PATCH 3/5] MDL-67353 favourites: Random DB id matching solved Depending on the tests executed it was possible to arrive to favourites tests with 2 "concepts" (say userid and contextid, or userid and itemid) having the same ID. Then, the array_diff() operations used by some mock stuff in the tests wrongly was returning matches by value, ultimately causing the test to fail. Now, the matching is performed using array_diff_assoc() that takes keys into considation when performing the match. --- favourites/tests/component_favourite_service_test.php | 10 +++++----- favourites/tests/user_favourite_service_test.php | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/favourites/tests/component_favourite_service_test.php b/favourites/tests/component_favourite_service_test.php index 78c43d242e0..b4440073aaa 100644 --- a/favourites/tests/component_favourite_service_test.php +++ b/favourites/tests/component_favourite_service_test.php @@ -88,7 +88,7 @@ class component_favourite_service_testcase extends advanced_testcase { // Check the mockstore for all objects with properties matching the key => val pairs in $criteria. foreach ($mockstore as $index => $mockrow) { $mockrowarr = (array)$mockrow; - if (array_diff($criteria, $mockrowarr) == []) { + if (array_diff_assoc($criteria, $mockrowarr) == []) { $returns[$index] = $mockrow; } } @@ -107,7 +107,7 @@ class component_favourite_service_testcase extends advanced_testcase { $crit = ['userid' => $userid, 'component' => $comp, 'itemtype' => $type, 'itemid' => $id, 'contextid' => $ctxid]; foreach ($mockstore as $fakerow) { $fakerowarr = (array)$fakerow; - if (array_diff($crit, $fakerowarr) == []) { + if (array_diff_assoc($crit, $fakerowarr) == []) { return $fakerow; } } @@ -133,7 +133,7 @@ class component_favourite_service_testcase extends advanced_testcase { // Check the mockstore for all objects with properties matching the key => val pairs in $criteria. foreach ($mockstore as $index => $mockrow) { $mockrowarr = (array)$mockrow; - if (array_diff($criteria, $mockrowarr) == []) { + if (array_diff_assoc($criteria, $mockrowarr) == []) { $count++; } } @@ -156,7 +156,7 @@ class component_favourite_service_testcase extends advanced_testcase { // Check the mockstore for all objects with properties matching the key => val pairs in $criteria. foreach ($mockstore as $index => $mockrow) { $mockrowarr = (array)$mockrow; - if (array_diff($criteria, $mockrowarr) == []) { + if (array_diff_assoc($criteria, $mockrowarr) == []) { unset($mockstore[$index]); } } @@ -169,7 +169,7 @@ class component_favourite_service_testcase extends advanced_testcase { foreach ($mockstore as $index => $mockrow) { $mockrowarr = (array)$mockrow; echo "Here"; - if (array_diff($criteria, $mockrowarr) == []) { + if (array_diff_assoc($criteria, $mockrowarr) == []) { return true; } } diff --git a/favourites/tests/user_favourite_service_test.php b/favourites/tests/user_favourite_service_test.php index 3483a33cd26..bc172afc9be 100644 --- a/favourites/tests/user_favourite_service_test.php +++ b/favourites/tests/user_favourite_service_test.php @@ -88,7 +88,7 @@ class user_favourite_service_testcase extends advanced_testcase { // Check the mockstore for all objects with properties matching the key => val pairs in $criteria. foreach ($mockstore as $index => $mockrow) { $mockrowarr = (array)$mockrow; - if (array_diff($criteria, $mockrowarr) == []) { + if (array_diff_assoc($criteria, $mockrowarr) == []) { $returns[$index] = $mockrow; } } @@ -107,7 +107,7 @@ class user_favourite_service_testcase extends advanced_testcase { $crit = ['userid' => $userid, 'component' => $comp, 'itemtype' => $type, 'itemid' => $id, 'contextid' => $ctxid]; foreach ($mockstore as $fakerow) { $fakerowarr = (array)$fakerow; - if (array_diff($crit, $fakerowarr) == []) { + if (array_diff_assoc($crit, $fakerowarr) == []) { return $fakerow; } } @@ -133,7 +133,7 @@ class user_favourite_service_testcase extends advanced_testcase { // Check the mockstore for all objects with properties matching the key => val pairs in $criteria. foreach ($mockstore as $index => $mockrow) { $mockrowarr = (array)$mockrow; - if (array_diff($criteria, $mockrowarr) == []) { + if (array_diff_assoc($criteria, $mockrowarr) == []) { $count++; } } @@ -156,7 +156,7 @@ class user_favourite_service_testcase extends advanced_testcase { // Check the mockstore for all objects with properties matching the key => val pairs in $criteria. foreach ($mockstore as $index => $mockrow) { $mockrowarr = (array)$mockrow; - if (array_diff($criteria, $mockrowarr) == []) { + if (array_diff_assoc($criteria, $mockrowarr) == []) { return true; } } From ccb5136b94061947571202c39d4ad94facb9eb1f Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sun, 12 Jan 2020 04:16:35 +0100 Subject: [PATCH 4/5] MDL-67353 filters: fix filterlib tests - move resetAfterTest() to test units. - remove setUp(), moving code to remove_all_filters_from_config(). - remove class properties, moving to return values of own setup_xxx() methods. --- lib/tests/filterlib_test.php | 326 +++++++++++++++++++++++------------ 1 file changed, 220 insertions(+), 106 deletions(-) diff --git a/lib/tests/filterlib_test.php b/lib/tests/filterlib_test.php index c3aab08cb91..8f8e5fa759a 100644 --- a/lib/tests/filterlib_test.php +++ b/lib/tests/filterlib_test.php @@ -33,23 +33,6 @@ require_once($CFG->libdir . '/filterlib.php'); * Test filters. */ class core_filterlib_testcase extends advanced_testcase { - private $syscontext; - private $childcontext; - private $childcontext2; - private $catcontext; - private $coursecontext; - private $course; - private $activity1context; - private $activity2context; - - protected function setUp() { - global $DB; - parent::setUp(); - - $this->resetAfterTest(); - $DB->delete_records('filter_active', array()); - $DB->delete_records('filter_config', array()); - } private function assert_only_one_filter_globally($filter, $state) { global $DB; @@ -79,6 +62,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_set_filter_globally_on() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. // Exercise SUT. filter_set_global_state('name', TEXTFILTER_ON); @@ -87,6 +72,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_set_filter_globally_off() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. // Exercise SUT. filter_set_global_state('name', TEXTFILTER_OFF); @@ -95,6 +82,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_set_filter_globally_disabled() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. // Exercise SUT. filter_set_global_state('name', TEXTFILTER_DISABLED); @@ -106,10 +95,13 @@ class core_filterlib_testcase extends advanced_testcase { * @expectedException coding_exception */ public function test_global_config_exception_on_invalid_state() { + $this->resetAfterTest(); filter_set_global_state('name', 0); } public function test_auto_sort_order() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. // Exercise SUT. filter_set_global_state('one', TEXTFILTER_DISABLED); @@ -119,6 +111,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_auto_sort_order_enabled() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. // Exercise SUT. filter_set_global_state('one', TEXTFILTER_ON); @@ -128,6 +122,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_update_existing_dont_duplicate() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. // Exercise SUT. filter_set_global_state('name', TEXTFILTER_ON); @@ -137,6 +133,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_update_reorder_down() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_global_state('one', TEXTFILTER_ON); filter_set_global_state('two', TEXTFILTER_ON); @@ -148,6 +146,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_update_reorder_up() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_global_state('one', TEXTFILTER_ON); filter_set_global_state('two', TEXTFILTER_ON); @@ -160,6 +160,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_auto_sort_order_change_to_enabled() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_global_state('one', TEXTFILTER_ON); filter_set_global_state('two', TEXTFILTER_DISABLED); @@ -171,6 +173,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_auto_sort_order_change_to_disabled() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_global_state('one', TEXTFILTER_ON); filter_set_global_state('two', TEXTFILTER_ON); @@ -182,6 +186,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_filter_get_global_states() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_global_state('one', TEXTFILTER_ON); filter_set_global_state('two', TEXTFILTER_OFF); @@ -216,6 +222,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_local_on() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Exercise SUT. filter_set_local_state('name', 123, TEXTFILTER_ON); // Validate. @@ -223,6 +231,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_local_off() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Exercise SUT. filter_set_local_state('name', 123, TEXTFILTER_OFF); // Validate. @@ -230,6 +240,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_local_inherit() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Exercise SUT. filter_set_local_state('name', 123, TEXTFILTER_INHERIT); // Validate. @@ -240,6 +252,7 @@ class core_filterlib_testcase extends advanced_testcase { * @expectedException coding_exception */ public function test_local_invalid_state_throws_exception() { + $this->resetAfterTest(); // Exercise SUT. filter_set_local_state('name', 123, -9999); } @@ -248,11 +261,14 @@ class core_filterlib_testcase extends advanced_testcase { * @expectedException coding_exception */ public function test_throws_exception_when_setting_global() { + $this->resetAfterTest(); // Exercise SUT. filter_set_local_state('name', context_system::instance()->id, TEXTFILTER_INHERIT); } public function test_local_inherit_deletes_existing() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_local_state('name', 123, TEXTFILTER_INHERIT); // Exercise SUT. @@ -276,6 +292,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_set_new_config() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Exercise SUT. filter_set_local_config('name', 123, 'settingname', 'An arbitrary value'); // Validate. @@ -283,6 +301,8 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_update_existing_config() { + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_local_config('name', 123, 'settingname', 'An arbitrary value'); // Exercise SUT. @@ -292,6 +312,7 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_filter_get_local_config() { + $this->resetAfterTest(); // Setup fixture. filter_set_local_config('name', 123, 'setting1', 'An arbitrary value'); filter_set_local_config('name', 123, 'setting2', 'Another arbitrary value'); @@ -306,22 +327,37 @@ class core_filterlib_testcase extends advanced_testcase { protected function setup_available_in_context_tests() { $course = $this->getDataGenerator()->create_course(array('category'=>1)); - $this->childcontext = context_coursecat::instance(1); - $this->childcontext2 = context_course::instance($course->id); - $this->syscontext = context_system::instance(); + $childcontext = context_coursecat::instance(1); + $childcontext2 = context_course::instance($course->id); + $syscontext = context_system::instance(); + + return [ + 'syscontext' => $syscontext, + 'childcontext' => $childcontext, + 'childcontext2' => $childcontext2 + ]; + } + + protected function remove_all_filters_from_config() { + global $DB; + $DB->delete_records('filter_active', array()); + $DB->delete_records('filter_config', array()); } private function assert_filter_list($expectedfilters, $filters) { - $this->setup_available_in_context_tests(); $this->assertEquals($expectedfilters, array_keys($filters), '', 0, 10, true); } public function test_globally_on_is_returned() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'syscontext' => $syscontext + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); // Exercise SUT. - $filters = filter_get_active_in_context($this->syscontext); + $filters = filter_get_active_in_context($syscontext); // Validate. $this->assert_filter_list(array('name'), $filters); // Check no config returned correctly. @@ -329,97 +365,128 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_globally_off_not_returned() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'childcontext2' => $childcontext2 + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_OFF); // Exercise SUT. - $filters = filter_get_active_in_context($this->childcontext2); + $filters = filter_get_active_in_context($childcontext2); // Validate. $this->assert_filter_list(array(), $filters); } public function test_globally_off_overridden() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'childcontext' => $childcontext, + 'childcontext2' => $childcontext2 + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_OFF); - filter_set_local_state('name', $this->childcontext->id, TEXTFILTER_ON); + filter_set_local_state('name', $childcontext->id, TEXTFILTER_ON); // Exercise SUT. - $filters = filter_get_active_in_context($this->childcontext2); + $filters = filter_get_active_in_context($childcontext2); // Validate. $this->assert_filter_list(array('name'), $filters); } public function test_globally_on_overridden() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'childcontext' => $childcontext, + 'childcontext2' => $childcontext2 + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); - filter_set_local_state('name', $this->childcontext->id, TEXTFILTER_OFF); + filter_set_local_state('name', $childcontext->id, TEXTFILTER_OFF); // Exercise SUT. - $filters = filter_get_active_in_context($this->childcontext2); + $filters = filter_get_active_in_context($childcontext2); // Validate. $this->assert_filter_list(array(), $filters); } public function test_globally_disabled_not_overridden() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'syscontext' => $syscontext, + 'childcontext' => $childcontext + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_DISABLED); - filter_set_local_state('name', $this->childcontext->id, TEXTFILTER_ON); + filter_set_local_state('name', $childcontext->id, TEXTFILTER_ON); // Exercise SUT. - $filters = filter_get_active_in_context($this->syscontext); + $filters = filter_get_active_in_context($syscontext); // Validate. $this->assert_filter_list(array(), $filters); } public function test_single_config_returned() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + [ + 'childcontext' => $childcontext + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); - filter_set_local_config('name', $this->childcontext->id, 'settingname', 'A value'); + filter_set_local_config('name', $childcontext->id, 'settingname', 'A value'); // Exercise SUT. - $filters = filter_get_active_in_context($this->childcontext); + $filters = filter_get_active_in_context($childcontext); // Validate. $this->assertEquals(array('settingname' => 'A value'), $filters['name']); } public function test_multi_config_returned() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + [ + 'childcontext' => $childcontext + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); - filter_set_local_config('name', $this->childcontext->id, 'settingname', 'A value'); - filter_set_local_config('name', $this->childcontext->id, 'anothersettingname', 'Another value'); + filter_set_local_config('name', $childcontext->id, 'settingname', 'A value'); + filter_set_local_config('name', $childcontext->id, 'anothersettingname', 'Another value'); // Exercise SUT. - $filters = filter_get_active_in_context($this->childcontext); + $filters = filter_get_active_in_context($childcontext); // Validate. $this->assertEquals(array('settingname' => 'A value', 'anothersettingname' => 'Another value'), $filters['name']); } public function test_config_from_other_context_not_returned() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + [ + 'childcontext' => $childcontext, + 'childcontext2' => $childcontext2 + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); - filter_set_local_config('name', $this->childcontext->id, 'settingname', 'A value'); - filter_set_local_config('name', $this->childcontext2->id, 'anothersettingname', 'Another value'); + filter_set_local_config('name', $childcontext->id, 'settingname', 'A value'); + filter_set_local_config('name', $childcontext2->id, 'anothersettingname', 'Another value'); // Exercise SUT. - $filters = filter_get_active_in_context($this->childcontext2); + $filters = filter_get_active_in_context($childcontext2); // Validate. $this->assertEquals(array('anothersettingname' => 'Another value'), $filters['name']); } public function test_config_from_other_filter_not_returned() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + [ + 'childcontext' => $childcontext + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); - filter_set_local_config('name', $this->childcontext->id, 'settingname', 'A value'); - filter_set_local_config('other', $this->childcontext->id, 'anothersettingname', 'Another value'); + filter_set_local_config('name', $childcontext->id, 'settingname', 'A value'); + filter_set_local_config('other', $childcontext->id, 'anothersettingname', 'Another value'); // Exercise SUT. - $filters = filter_get_active_in_context($this->childcontext); + $filters = filter_get_active_in_context($childcontext); // Validate. $this->assertEquals(array('settingname' => 'A value'), $filters['name']); } protected function assert_one_available_filter($filter, $localstate, $inheritedstate, $filters) { - $this->setup_available_in_context_tests(); $this->assertEquals(1, count($filters), 'More than one record returned %s.'); $rec = $filters[$filter]; unset($rec->id); @@ -431,34 +498,47 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_available_in_context_localoverride() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'childcontext' => $childcontext + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); - filter_set_local_state('name', $this->childcontext->id, TEXTFILTER_OFF); + filter_set_local_state('name', $childcontext->id, TEXTFILTER_OFF); // Exercise SUT. - $filters = filter_get_available_in_context($this->childcontext); + $filters = filter_get_available_in_context($childcontext); // Validate. $this->assert_one_available_filter('name', TEXTFILTER_OFF, TEXTFILTER_ON, $filters); } public function test_available_in_context_nolocaloverride() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'childcontext' => $childcontext, + 'childcontext2' => $childcontext2 + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); - filter_set_local_state('name', $this->childcontext->id, TEXTFILTER_OFF); + filter_set_local_state('name', $childcontext->id, TEXTFILTER_OFF); // Exercise SUT. - $filters = filter_get_available_in_context($this->childcontext2); + $filters = filter_get_available_in_context($childcontext2); // Validate. $this->assert_one_available_filter('name', TEXTFILTER_INHERIT, TEXTFILTER_OFF, $filters); } public function test_available_in_context_disabled_not_returned() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'childcontext' => $childcontext + ] = $this->setup_available_in_context_tests(); // Setup fixture. filter_set_global_state('name', TEXTFILTER_DISABLED); - filter_set_local_state('name', $this->childcontext->id, TEXTFILTER_ON); + filter_set_local_state('name', $childcontext->id, TEXTFILTER_ON); // Exercise SUT. - $filters = filter_get_available_in_context($this->childcontext); + $filters = filter_get_available_in_context($childcontext); // Validate. $this->assertEquals(array(), $filters); } @@ -467,23 +547,34 @@ class core_filterlib_testcase extends advanced_testcase { * @expectedException coding_exception */ public function test_available_in_context_exception_with_syscontext() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + [ + 'syscontext' => $syscontext + ] = $this->setup_available_in_context_tests(); // Exercise SUT. - filter_get_available_in_context($this->syscontext); + filter_get_available_in_context($syscontext); } protected function setup_preload_activities_test() { - $this->syscontext = context_system::instance(); - $this->catcontext = context_coursecat::instance(1); - $this->course = $this->getDataGenerator()->create_course(array('category'=>1)); - $this->coursecontext = context_course::instance($this->course->id); - $page1 = $this->getDataGenerator()->create_module('page', array('course'=>$this->course->id)); - $this->activity1context = context_module::instance($page1->cmid); - $page2 = $this->getDataGenerator()->create_module('page', array('course'=>$this->course->id)); - $this->activity2context = context_module::instance($page2->cmid); + $syscontext = context_system::instance(); + $catcontext = context_coursecat::instance(1); + $course = $this->getDataGenerator()->create_course(array('category' => 1)); + $coursecontext = context_course::instance($course->id); + $page1 = $this->getDataGenerator()->create_module('page', array('course' => $course->id)); + $activity1context = context_module::instance($page1->cmid); + $page2 = $this->getDataGenerator()->create_module('page', array('course' => $course->id)); + $activity2context = context_module::instance($page2->cmid); + return [ + 'syscontext' => $syscontext, + 'catcontext' => $catcontext, + 'course' => $course, + 'coursecontext' => $coursecontext, + 'activity1context' => $activity1context, + 'activity2context' => $activity2context + ]; } - private function assert_matches($modinfo) { + private function assert_matches($modinfo, $activity1context, $activity2context) { global $FILTERLIB_PRIVATE, $DB; // Use preload cache... @@ -492,16 +583,16 @@ class core_filterlib_testcase extends advanced_testcase { // Get data and check no queries are made. $before = $DB->perf_get_reads(); - $plfilters1 = filter_get_active_in_context($this->activity1context); - $plfilters2 = filter_get_active_in_context($this->activity2context); + $plfilters1 = filter_get_active_in_context($activity1context); + $plfilters2 = filter_get_active_in_context($activity2context); $after = $DB->perf_get_reads(); $this->assertEquals($before, $after); // Repeat without cache and check it makes queries now. $FILTERLIB_PRIVATE = new stdClass; $before = $DB->perf_get_reads(); - $filters1 = filter_get_active_in_context($this->activity1context); - $filters2 = filter_get_active_in_context($this->activity2context); + $filters1 = filter_get_active_in_context($activity1context); + $filters2 = filter_get_active_in_context($activity2context); $after = $DB->perf_get_reads(); $this->assertTrue($after > $before); @@ -511,60 +602,69 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_preload() { - $this->setup_preload_activities_test(); + $this->resetAfterTest(); + [ + 'catcontext' => $catcontext, + 'course' => $course, + 'coursecontext' => $coursecontext, + 'activity1context' => $activity1context, + 'activity2context' => $activity2context + ] = $this->setup_preload_activities_test(); // Get course and modinfo. - $modinfo = new course_modinfo($this->course, 2); + $modinfo = new course_modinfo($course, 2); // Note: All the tests in this function check that the result from the // preloaded cache is the same as the result from calling the standard // function without preloading. // Initially, check with no filters enabled. - $this->assert_matches($modinfo); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Enable filter globally, check. filter_set_global_state('name', TEXTFILTER_ON); - $this->assert_matches($modinfo); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Disable for activity 2. - filter_set_local_state('name', $this->activity2context->id, TEXTFILTER_OFF); - $this->assert_matches($modinfo); + filter_set_local_state('name', $activity2context->id, TEXTFILTER_OFF); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Disable at category. - filter_set_local_state('name', $this->catcontext->id, TEXTFILTER_OFF); - $this->assert_matches($modinfo); + filter_set_local_state('name', $catcontext->id, TEXTFILTER_OFF); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Enable for activity 1. - filter_set_local_state('name', $this->activity1context->id, TEXTFILTER_ON); - $this->assert_matches($modinfo); + filter_set_local_state('name', $activity1context->id, TEXTFILTER_ON); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Disable globally. filter_set_global_state('name', TEXTFILTER_DISABLED); - $this->assert_matches($modinfo); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Add another 2 filters. filter_set_global_state('frog', TEXTFILTER_ON); filter_set_global_state('zombie', TEXTFILTER_ON); - $this->assert_matches($modinfo); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Disable random one of these in each context. - filter_set_local_state('zombie', $this->activity1context->id, TEXTFILTER_OFF); - filter_set_local_state('frog', $this->activity2context->id, TEXTFILTER_OFF); - $this->assert_matches($modinfo); + filter_set_local_state('zombie', $activity1context->id, TEXTFILTER_OFF); + filter_set_local_state('frog', $activity2context->id, TEXTFILTER_OFF); + $this->assert_matches($modinfo, $activity1context, $activity2context); // Now do some filter options. - filter_set_local_config('name', $this->activity1context->id, 'a', 'x'); - filter_set_local_config('zombie', $this->activity1context->id, 'a', 'y'); - filter_set_local_config('frog', $this->activity1context->id, 'a', 'z'); + filter_set_local_config('name', $activity1context->id, 'a', 'x'); + filter_set_local_config('zombie', $activity1context->id, 'a', 'y'); + filter_set_local_config('frog', $activity1context->id, 'a', 'z'); // These last two don't do anything as they are not at final level but I // thought it would be good to have that verified in test. - filter_set_local_config('frog', $this->coursecontext->id, 'q', 'x'); - filter_set_local_config('frog', $this->catcontext->id, 'q', 'z'); - $this->assert_matches($modinfo); + filter_set_local_config('frog', $coursecontext->id, 'q', 'x'); + filter_set_local_config('frog', $catcontext->id, 'q', 'z'); + $this->assert_matches($modinfo, $activity1context, $activity2context); } public function test_filter_delete_all_for_filter() { global $DB; + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); @@ -588,6 +688,8 @@ class core_filterlib_testcase extends advanced_testcase { public function test_filter_delete_all_for_context() { global $DB; + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. // Setup fixture. filter_set_global_state('name', TEXTFILTER_ON); @@ -606,6 +708,7 @@ class core_filterlib_testcase extends advanced_testcase { public function test_set() { global $CFG; + $this->resetAfterTest(); $this->assertFileExists("$CFG->dirroot/filter/emailprotect"); // Any standard filter. $this->assertFileExists("$CFG->dirroot/filter/tidy"); // Any standard filter. @@ -631,6 +734,7 @@ class core_filterlib_testcase extends advanced_testcase { public function test_unset_to_empty() { global $CFG; + $this->resetAfterTest(); $this->assertFileExists("$CFG->dirroot/filter/tidy"); // Any standard filter. @@ -646,6 +750,7 @@ class core_filterlib_testcase extends advanced_testcase { public function test_unset_multi() { global $CFG; + $this->resetAfterTest(); $this->assertFileExists("$CFG->dirroot/filter/emailprotect"); // Any standard filter. $this->assertFileExists("$CFG->dirroot/filter/tidy"); // Any standard filter. @@ -662,6 +767,7 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_filter_manager_instance() { + $this->resetAfterTest(); set_config('perfdebug', 7); filter_manager::reset_caches(); @@ -677,18 +783,26 @@ class core_filterlib_testcase extends advanced_testcase { } public function test_filter_get_globally_enabled_default() { + $this->resetAfterTest(); $enabledfilters = filter_get_globally_enabled(); $this->assertArrayNotHasKey('glossary', $enabledfilters); } public function test_filter_get_globally_enabled_after_change() { + $this->resetAfterTest(); filter_set_global_state('glossary', TEXTFILTER_ON); $enabledfilters = filter_get_globally_enabled(); $this->assertArrayHasKey('glossary', $enabledfilters); } public function test_filter_get_globally_enabled_filters_with_config() { - $this->setup_available_in_context_tests(); + $this->resetAfterTest(); + $this->remove_all_filters_from_config(); // Remove all filters. + [ + 'syscontext' => $syscontext, + 'childcontext' => $childcontext + ] = $this->setup_available_in_context_tests(); + $this->remove_all_filters_from_config(); // Remove all filters. // Set few filters. filter_set_global_state('one', TEXTFILTER_ON); @@ -696,18 +810,18 @@ class core_filterlib_testcase extends advanced_testcase { filter_set_global_state('two', TEXTFILTER_DISABLED); // Set global config. - filter_set_local_config('one', $this->syscontext->id, 'test1a', 'In root'); - filter_set_local_config('one', $this->syscontext->id, 'test1b', 'In root'); - filter_set_local_config('two', $this->syscontext->id, 'test2a', 'In root'); - filter_set_local_config('two', $this->syscontext->id, 'test2b', 'In root'); + filter_set_local_config('one', $syscontext->id, 'test1a', 'In root'); + filter_set_local_config('one', $syscontext->id, 'test1b', 'In root'); + filter_set_local_config('two', $syscontext->id, 'test2a', 'In root'); + filter_set_local_config('two', $syscontext->id, 'test2b', 'In root'); // Set child config. - filter_set_local_config('one', $this->childcontext->id, 'test1a', 'In child'); - filter_set_local_config('one', $this->childcontext->id, 'test1b', 'In child'); - filter_set_local_config('two', $this->childcontext->id, 'test2a', 'In child'); - filter_set_local_config('two', $this->childcontext->id, 'test2b', 'In child'); - filter_set_local_config('three', $this->childcontext->id, 'test3a', 'In child'); - filter_set_local_config('three', $this->childcontext->id, 'test3b', 'In child'); + filter_set_local_config('one', $childcontext->id, 'test1a', 'In child'); + filter_set_local_config('one', $childcontext->id, 'test1b', 'In child'); + filter_set_local_config('two', $childcontext->id, 'test2a', 'In child'); + filter_set_local_config('two', $childcontext->id, 'test2b', 'In child'); + filter_set_local_config('three', $childcontext->id, 'test3a', 'In child'); + filter_set_local_config('three', $childcontext->id, 'test3b', 'In child'); // Check. $actual = filter_get_globally_enabled_filters_with_config(); From b2dfaa5d3a7e355fa9741de3dbda0c0cc4fe4c5f Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 29 Jan 2020 23:43:57 +0100 Subject: [PATCH 5/5] MDL-67353 course: move form tearDown() to reset_all_data() We are already cleaning all sort of caches, statics, singletons there and it's better to ensure they are always reset to avoid myterious failures @ distance. --- course/classes/customfield/course_handler.php | 2 +- customfield/tests/api_test.php | 8 -------- customfield/tests/category_controller_test.php | 8 -------- customfield/tests/data_controller_test.php | 8 -------- customfield/tests/field_controller_test.php | 8 -------- customfield/tests/generator_test.php | 8 -------- customfield/tests/privacy_test.php | 8 -------- lib/phpunit/classes/util.php | 3 +++ 8 files changed, 4 insertions(+), 49 deletions(-) diff --git a/course/classes/customfield/course_handler.php b/course/classes/customfield/course_handler.php index 5deeb4737ff..dede44dcbb9 100644 --- a/course/classes/customfield/course_handler.php +++ b/course/classes/customfield/course_handler.php @@ -71,7 +71,7 @@ class course_handler extends \core_customfield\handler { /** * Run reset code after unit tests to reset the singleton usage. */ - public static function reset_after_test(): void { + public static function reset_caches(): void { if (!PHPUNIT_TEST) { throw new \coding_exception('This feature is only intended for use in unit tests'); } diff --git a/customfield/tests/api_test.php b/customfield/tests/api_test.php index 434eb355bae..e76d410e05b 100644 --- a/customfield/tests/api_test.php +++ b/customfield/tests/api_test.php @@ -37,14 +37,6 @@ use \core_customfield\category_controller; */ class core_customfield_api_testcase extends advanced_testcase { - /** - * Tear down to reset the singleton after each test. - */ - public function tearDown() { - core_course\customfield\course_handler::reset_after_test(); - parent::tearDown(); - } - /** * Get generator. * diff --git a/customfield/tests/category_controller_test.php b/customfield/tests/category_controller_test.php index 210a9669ba3..04ce151e65e 100644 --- a/customfield/tests/category_controller_test.php +++ b/customfield/tests/category_controller_test.php @@ -36,14 +36,6 @@ use \core_customfield\field_controller; */ class core_customfield_category_controller_testcase extends advanced_testcase { - /** - * Tear down to reset the singleton after each test. - */ - public function tearDown() { - core_course\customfield\course_handler::reset_after_test(); - parent::tearDown(); - } - /** * Get generator. * diff --git a/customfield/tests/data_controller_test.php b/customfield/tests/data_controller_test.php index ab5b9da3806..d125c5c9c21 100644 --- a/customfield/tests/data_controller_test.php +++ b/customfield/tests/data_controller_test.php @@ -34,14 +34,6 @@ use core_customfield\data_controller; */ class core_customfield_data_controller_testcase extends advanced_testcase { - /** - * Tear down to reset the singleton after each test. - */ - public function tearDown() { - core_course\customfield\course_handler::reset_after_test(); - parent::tearDown(); - } - /** * Get generator. * diff --git a/customfield/tests/field_controller_test.php b/customfield/tests/field_controller_test.php index ff26572fe05..a642919e262 100644 --- a/customfield/tests/field_controller_test.php +++ b/customfield/tests/field_controller_test.php @@ -38,14 +38,6 @@ use \core_customfield\field_controller; */ class core_customfield_field_controller_testcase extends advanced_testcase { - /** - * Tear down to reset the singleton after each test. - */ - public function tearDown() { - core_course\customfield\course_handler::reset_after_test(); - parent::tearDown(); - } - /** * Get generator. * diff --git a/customfield/tests/generator_test.php b/customfield/tests/generator_test.php index 38d8ebb9f84..c5335c37d9f 100644 --- a/customfield/tests/generator_test.php +++ b/customfield/tests/generator_test.php @@ -35,14 +35,6 @@ defined('MOODLE_INTERNAL') || die(); */ class core_customfield_generator_testcase extends advanced_testcase { - /** - * Tear down to reset the singleton after each test. - */ - public function tearDown() { - core_course\customfield\course_handler::reset_after_test(); - parent::tearDown(); - } - /** * Get generator * @return core_customfield_generator diff --git a/customfield/tests/privacy_test.php b/customfield/tests/privacy_test.php index bdb5d374b72..4a63085fb3f 100644 --- a/customfield/tests/privacy_test.php +++ b/customfield/tests/privacy_test.php @@ -38,14 +38,6 @@ use core_customfield\privacy\provider; */ class core_customfield_privacy_testcase extends provider_testcase { - /** - * Tear down to reset the singleton after each test. - */ - public function tearDown() { - core_course\customfield\course_handler::reset_after_test(); - parent::tearDown(); - } - /** * Generate data. * diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index e69ae8cecdd..2605f6fde15 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -252,6 +252,9 @@ class phpunit_util extends testing_util { if (class_exists('\core\update\checker')) { \core\update\checker::reset_caches(true); } + if (class_exists('\core_course\customfield\course_handler')) { + \core_course\customfield\course_handler::reset_caches(); + } // Clear static cache within restore. if (class_exists('restore_section_structure_step')) {