From a999b12e02e04cae2582c2bc4d04e27b415f2ead Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 3 Feb 2022 11:09:50 +0800 Subject: [PATCH 1/3] MDL-72246 core_completion: Stop testing the inner workings of get_data() --- lib/tests/completionlib_test.php | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index 00ae923f469..e35f5be614f 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -760,26 +760,6 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(0, $result->id); } - // Check caching. - $key = "{$user->id}_{$this->course->id}"; - $cache = cache::make('core', 'completion'); - if ($iscached) { - // If we expect this to be cached, then fetching the result must match the cached data. - $this->assertEquals($result, (object)$cache->get($key)[$cm->id]); - - // Check cached data for other course modules in the course. - // The sample module created in setup_data() should suffice to confirm this. - $othercm = get_coursemodule_from_instance('forum', $this->module1->id); - if ($wholecourse) { - $this->assertArrayHasKey($othercm->id, $cache->get($key)); - } else { - $this->assertArrayNotHasKey($othercm->id, $cache->get($key)); - } - } else { - // Otherwise, this should not be cached. - $this->assertFalse($cache->get($key)); - } - // Check that we are including relevant completion data for the module. if (!$wholecourse) { $this->assertTrue(property_exists($result, 'viewed')); From adc61dddc32222323410eee94036d1d14058caef Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 3 Feb 2022 13:27:24 +0800 Subject: [PATCH 2/3] MDL-72246 core_completion: Test coverage improvements for completion --- completion/tests/coverage.php | 47 +++++++++++++++++ lib/tests/completionlib_test.php | 91 +++++++++++++++++++++++++++++++- 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 completion/tests/coverage.php diff --git a/completion/tests/coverage.php b/completion/tests/coverage.php new file mode 100644 index 00000000000..2a758a79d66 --- /dev/null +++ b/completion/tests/coverage.php @@ -0,0 +1,47 @@ +. + +defined('MOODLE_INTERNAL') || die(); + +/** + * Coverage information for the core_completion. + * + * @package core + * @category phpunit + * @copyright 2022 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +/** + * Coverage information for the core subsystem. + * + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +return new class extends phpunit_coverage_info { + /** @var array The list of folders relative to the plugin root to include in coverage generation. */ + protected $includelistfolders = [ + 'criteria', + ]; + + /** @var array The list of files relative to the plugin root to include in coverage generation. */ + protected $includelistfiles = [ + 'completion_aggregation.php', + 'completion_completion.php', + 'completion_criteria_completion.php', + 'data_object.php' + ]; +}; diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index e35f5be614f..b393c90dbe8 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -29,7 +29,17 @@ defined('MOODLE_INTERNAL') || die(); global $CFG; require_once($CFG->libdir.'/completionlib.php'); -class core_completionlib_testcase extends advanced_testcase { +/** + * Completion tests. + * + * @package core_completion + * @category phpunit + * @copyright 2008 Sam Marshall + * @copyright 2013 Frédéric Massart + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \completion_info + */ +class completionlib_test extends advanced_testcase { protected $course; protected $user; protected $module1; @@ -90,6 +100,10 @@ class core_completionlib_testcase extends advanced_testcase { parent::assertEquals($expected, $actual, $message, $delta, $maxDepth, $canonicalize, $ignoreCase); } + /** + * @covers ::is_enabled_for_site + * @covers ::is_enabled + */ public function test_is_enabled() { global $CFG; $this->mock_setup(); @@ -125,6 +139,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(COMPLETION_TRACKING_AUTOMATIC, $c->is_enabled($cm)); } + /** + * @covers ::update_state + */ public function test_update_state() { $this->mock_setup(); @@ -348,6 +365,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param int $completionusegrade * @param string $unsetfield * @param int $expectedstate + * @covers ::internal_get_state */ public function test_internal_get_state(int $completionview, int $completionusegrade, string $unsetfield, int $expectedstate) { $this->setup_data(); @@ -379,6 +397,7 @@ class core_completionlib_testcase extends advanced_testcase { * Provider for the test_internal_get_state_with_grade_criteria. * * @return array + * @covers ::internal_get_state */ public function test_internal_get_state_with_grade_criteria_provider() { return [ @@ -432,6 +451,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param array $completioncriteria The completion criteria to use * @param int|null $studentgrade Grade to assign to student * @param int $expectedstate Expected completion state + * @covers ::internal_get_state */ public function test_internal_get_state_with_grade_criteria(array $completioncriteria, ?int $studentgrade, int $expectedstate) { $this->setup_data(); @@ -472,6 +492,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Covers the case where internal_get_state() is being called for a user different from the logged in user. + * + * @covers ::internal_get_state */ public function test_internal_get_state_with_different_user() { $this->setup_data(); @@ -514,6 +536,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for internal_get_state() for an activity that supports custom completion. + * + * @covers ::internal_get_state */ public function test_internal_get_state_with_custom_completion() { $this->setup_data(); @@ -540,6 +564,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(COMPLETION_COMPLETE, $completion); } + /** + * @covers ::set_module_viewed + */ public function test_set_module_viewed() { $this->mock_setup(); @@ -595,6 +622,9 @@ class core_completionlib_testcase extends advanced_testcase { $c->set_module_viewed($cm, 1337); } + /** + * @covers ::count_user_data + */ public function test_count_user_data() { global $DB; $this->mock_setup(); @@ -611,6 +641,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(666, $c->count_user_data($cm)); } + /** + * @covers ::delete_all_state + */ public function test_delete_all_state() { global $DB; $this->mock_setup(); @@ -628,6 +661,9 @@ class core_completionlib_testcase extends advanced_testcase { $c->delete_all_state($cm); } + /** + * @covers ::reset_all_state + */ public function test_reset_all_state() { global $DB; $this->mock_setup(); @@ -703,6 +739,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param bool $sameuser Whether the user calling get_data() is the user itself. * @param bool $hasrecord Whether to create a course_modules_completion record. * @param int $completion The completion state expected. + * @covers ::get_data */ public function test_get_data(bool $wholecourse, bool $sameuser, bool $hasrecord, int $completion) { global $DB; @@ -769,6 +806,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Tests for completion_info::get_other_cm_completion_data(). + * + * @covers ::get_other_cm_completion_data */ public function test_get_other_cm_completion_data() { global $DB; @@ -842,6 +881,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEmpty($choice2completiondata); } + /** + * @covers ::internal_set_data + */ public function test_internal_set_data() { global $DB; $this->setup_data(); @@ -938,6 +980,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals($this->user->id, reset($actual)->userid); } + /** + * @covers ::get_progress_all + */ public function test_get_progress_all_few() { global $DB; $this->mock_setup(); @@ -972,6 +1017,9 @@ class core_completionlib_testcase extends advanced_testcase { ), $c->get_progress_all(false)); } + /** + * @covers ::get_progress_all + */ public function test_get_progress_all_lots() { global $DB; $this->mock_setup(); @@ -1027,6 +1075,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertCount(count($tracked), $result); } + /** + * @covers ::inform_grade_changed + */ public function test_inform_grade_changed() { $this->mock_setup(); @@ -1095,6 +1146,9 @@ class core_completionlib_testcase extends advanced_testcase { $c->inform_grade_changed($cm, $item, $grade, true); } + /** + * @covers ::internal_get_grade_state + */ public function test_internal_get_grade_state() { $this->mock_setup(); @@ -1137,6 +1191,9 @@ class core_completionlib_testcase extends advanced_testcase { completion_info::internal_get_grade_state($item, $grade)); } + /** + * @test ::get_activities + */ public function test_get_activities() { global $CFG; $this->resetAfterTest(); @@ -1178,6 +1235,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertFalse(isset($activities[$data2->cmid])); } + /** + * @test ::has_activities + */ public function test_has_activities() { global $CFG; $this->resetAfterTest(); @@ -1203,7 +1263,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test that data is cleaned up when we delete courses that are set as completion criteria for other courses * - * @return void + * @covers ::delete_course_completion_data + * @covers ::delete_all_completion_data */ public function test_course_delete_prerequisite() { global $DB; @@ -1240,6 +1301,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course module completion update event. + * + * @covers \core\event\course_module_completion_updated */ public function test_course_module_completion_updated_event() { global $USER, $CFG; @@ -1277,6 +1340,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course completed event. + * + * @covers \core\event\course_completed */ public function test_course_completed_event() { global $USER; @@ -1306,6 +1371,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course completed message. + * + * @covers \core\event\course_completed */ public function test_course_completed_message() { $this->setup_data(); @@ -1332,6 +1399,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course completed event. + * + * @covers \core\event\course_completion_updated */ public function test_course_completion_updated_event() { $this->setup_data(); @@ -1358,6 +1427,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEventLegacyLogData($expectedlegacylog, $event); } + /** + * @covers \completion_can_view_data + */ public function test_completion_can_view_data() { $this->setup_data(); @@ -1393,6 +1465,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param int|null $passinggrade Passing grade to set for the test activity. * @param string|null $expectedexception Expected exception. * @param int|null $expectedresult The expected completion status. + * @covers ::get_grade_completion */ public function test_get_grade_completion(bool $completionusegrade, bool $hasgrade, ?int $passinggrade, ?string $expectedexception, ?int $expectedresult) { @@ -1425,6 +1498,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test the return value for cases when the activity module does not have associated grade_item. + * + * @covers ::get_grade_completion */ public function test_get_grade_completion_without_grade_item() { global $DB; @@ -1463,6 +1538,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for aggregate_completions(). + * + * @covers \aggregate_completions */ public function test_aggregate_completions() { global $DB; @@ -1557,6 +1634,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::_save(). + * + * @covers \completion_completion::_save */ public function test_save() { global $DB; @@ -1601,6 +1680,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::mark_enrolled(). + * + * @covers \completion_completion::mark_enrolled */ public function test_mark_enrolled() { global $DB; @@ -1642,6 +1723,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::mark_inprogress(). + * + * @covers \completion_completion::mark_inprogress */ public function test_mark_inprogress() { global $DB; @@ -1683,6 +1766,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::mark_complete(). + * + * @covers \completion_completion::mark_complete */ public function test_mark_complete() { global $DB; @@ -1723,6 +1808,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_criteria_completion::mark_complete(). + * + * @covers \completion_criteria_completion::mark_complete */ public function test_criteria_mark_complete() { global $DB; From 70cc41eec88bfe26e45bab84d8d3996f48eab881 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Wed, 2 Feb 2022 16:10:59 +0800 Subject: [PATCH 3/3] MDL-72246 core_completion: Only fetch other completion data on requested Prior to this patch, the code was fetching all completion data for all activities in a course, even when the activity was not requested. This leads to recursion issues as the data has not been added to the cache before this operation occurs. To handle this situation, only the requested CM is fetched in full, and a boolean flag is used to store whether the full data has been fetched. When returning a partially fetched value from the cache, the flag is used to determine whether more data must be fetched, and the cache updated. The flag is filtered out before the value is returned. Note: Many of the tests were updated as these were inspecting private features of the API which should not really be tested. --- lib/completionlib.php | 67 ++++++++++++++++--------- lib/tests/completionlib_test.php | 85 ++++++++++++++++++++++++++++++-- 2 files changed, 127 insertions(+), 25 deletions(-) diff --git a/lib/completionlib.php b/lib/completionlib.php index 77c34bad9cf..94a9271327b 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -1024,6 +1024,20 @@ class completion_info { $userid = $USER->id; } + // Some call completion_info::get_data and pass $cm as an object with ID only. Make sure course is set as well. + if ($cm instanceof stdClass && !isset($cm->course)) { + $cm->course = $this->course_id; + } + // Make sure we're working on a cm_info object. + $cminfo = cm_info::create($cm, $userid); + + // Create an anonymous function to remove the 'other_cm_completion_data_fetched' key. + $returnfilteredvalue = function(array $value): stdClass { + return (object) array_filter($value, function(string $key): bool { + return $key !== 'other_cm_completion_data_fetched'; + }, ARRAY_FILTER_USE_KEY); + }; + // See if requested data is present in cache (use cache for current user only). $usecache = $userid == $USER->id; $cacheddata = array(); @@ -1036,19 +1050,22 @@ class completion_info { if ($cacheddata['cacherev'] != $this->course->cacherev) { // Course structure has been changed since the last caching, forget the cache. $cacheddata = array(); - } else if (isset($cacheddata[$cm->id])) { - return (object)$cacheddata[$cm->id]; + } else if (isset($cacheddata[$cminfo->id])) { + $data = (array) $cacheddata[$cminfo->id]; + if (empty($data['other_cm_completion_data_fetched'])) { + $data += $this->get_other_cm_completion_data($cminfo, $userid); + $data['other_cm_completion_data_fetched'] = true; + + // Put in cache. + $cacheddata[$cminfo->id] = $data; + $completioncache->set($key, $cacheddata); + } + + return $returnfilteredvalue($cacheddata[$cminfo->id]); } } } - // Some call completion_info::get_data and pass $cm as an object with ID only. Make sure course is set as well. - if ($cm instanceof stdClass && !isset($cm->course)) { - $cm->course = $this->course_id; - } - // Make sure we're working on a cm_info object. - $cminfo = cm_info::create($cm, $userid); - // Default data to return when no completion data is found. $defaultdata = [ 'id' => 0, @@ -1086,12 +1103,9 @@ class completion_info { $cacheddata[$data->cmid] = $defaultdata; $cacheddata[$data->cmid]['coursemoduleid'] = $data->cmid; } else { - $cacheddata[$data->cmid] = (array) $data; + unset($data->cmid); + $cacheddata[$data->coursemoduleid] = (array) $data; } - - // Add the other completion data for this user in this module instance. - $othercminfo = $cminfos[$data->cmid]; - $cacheddata[$othercminfo->id] += $this->get_other_cm_completion_data($othercminfo, $userid); } if (!isset($cacheddata[$cminfo->id])) { @@ -1099,6 +1113,7 @@ class completion_info { $this->internal_systemerror($errormessage); } + $data = $cacheddata[$cminfo->id]; } else { // Get single record $data = $DB->get_record('course_modules_completion', array('coursemoduleid' => $cminfo->id, 'userid' => $userid)); @@ -1108,18 +1123,24 @@ class completion_info { // Row not present counts as 'not complete'. $data = $defaultdata; } - // Fill the other completion data for this user in this module instance. - $data += $this->get_other_cm_completion_data($cminfo, $userid); - // Put in cache + // Put in cache. $cacheddata[$cminfo->id] = $data; } + // Fill the other completion data for this user in this module instance. + $data += $this->get_other_cm_completion_data($cminfo, $userid); + $data['other_cm_completion_data_fetched'] = true; + + // Put in cache + $cacheddata[$cminfo->id] = $data; + if ($usecache) { $cacheddata['cacherev'] = $this->course->cacherev; $completioncache->set($key, $cacheddata); } - return (object)$cacheddata[$cminfo->id]; + + return $returnfilteredvalue($cacheddata[$cminfo->id]); } /** @@ -1240,6 +1261,7 @@ class completion_info { $cmcontext = context_module::instance($data->coursemoduleid); $completioncache = cache::make('core', 'completion'); + $cachekey = "{$data->userid}_{$cm->course}"; if ($data->userid == $USER->id) { // Fetch other completion data to cache (e.g. require grade completion status, custom completion rule statues). $cminfo = cm_info::create($cm, $data->userid); // Make sure we're working on a cm_info object. @@ -1249,18 +1271,19 @@ class completion_info { } // Update module completion in user's cache. - if (!($cachedata = $completioncache->get($data->userid . '_' . $cm->course)) + if (!($cachedata = $completioncache->get($cachekey)) || $cachedata['cacherev'] != $this->course->cacherev) { $cachedata = array('cacherev' => $this->course->cacherev); } - $cachedata[$cm->id] = $data; - $completioncache->set($data->userid . '_' . $cm->course, $cachedata); + $cachedata[$cm->id] = (array) $data; + $cachedata[$cm->id]['other_cm_completion_data_fetched'] = true; + $completioncache->set($cachekey, $cachedata); // reset modinfo for user (no need to call rebuild_course_cache()) get_fast_modinfo($cm->course, 0, true); } else { // Remove another user's completion cache for this course. - $completioncache->delete($data->userid . '_' . $cm->course); + $completioncache->delete($cachekey); } // For single user actions the code must reevaluate some completion state instantly, see MDL-32103. diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index b393c90dbe8..ecd75696b2f 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -89,6 +89,10 @@ class completionlib_test extends advanced_testcase { public static function assertEquals($expected, $actual, string $message = '', float $delta = 0, int $maxDepth = 10, bool $canonicalize = false, bool $ignoreCase = false): void { // Nasty cheating hack: prevent random failures on timemodified field. + if (is_array($actual) && (is_object($expected) || is_array($expected))) { + $actual = (object) $actual; + $expected = (object) $expected; + } if (is_object($expected) and is_object($actual)) { if (property_exists($expected, 'timemodified') and property_exists($actual, 'timemodified')) { if ($expected->timemodified + 1 == $actual->timemodified) { @@ -804,6 +808,70 @@ class completionlib_test extends advanced_testcase { } } + /** + * @covers ::get_data + */ + public function test_get_data_successive_calls(): void { + global $DB; + + $this->setup_data(); + $this->setUser($this->user); + + $choicegenerator = $this->getDataGenerator()->get_plugin_generator('mod_choice'); + $choice = $choicegenerator->create_instance([ + 'course' => $this->course->id, + 'completion' => COMPLETION_TRACKING_AUTOMATIC, + 'completionview' => true, + 'completionsubmit' => true, + ]); + + $cm = get_coursemodule_from_instance('choice', $choice->id); + + // Let's manually create a course completion record instead of going through the hoops to complete an activity. + $cmcompletionrecord = (object) [ + 'coursemoduleid' => $cm->id, + 'userid' => $this->user->id, + 'completionstate' => COMPLETION_NOT_VIEWED, + 'viewed' => 0, + 'overrideby' => null, + 'timemodified' => 0, + ]; + $DB->insert_record('course_modules_completion', $cmcompletionrecord); + + // Mock other completion data. + $completioninfo = new completion_info($this->course); + + $modinfo = get_fast_modinfo($this->course); + $results = []; + foreach ($modinfo->cms as $testcm) { + $result = $completioninfo->get_data($testcm, true); + $this->assertTrue(property_exists($result, 'id')); + $this->assertTrue(property_exists($result, 'coursemoduleid')); + $this->assertTrue(property_exists($result, 'userid')); + $this->assertTrue(property_exists($result, 'completionstate')); + $this->assertTrue(property_exists($result, 'viewed')); + $this->assertTrue(property_exists($result, 'overrideby')); + $this->assertTrue(property_exists($result, 'timemodified')); + $this->assertFalse(property_exists($result, 'other_cm_completion_data_fetched')); + + $this->assertEquals($testcm->id, $result->coursemoduleid); + $this->assertEquals($this->user->id, $result->userid); + $this->assertEquals(0, $result->viewed); + + $results[$testcm->id] = $result; + } + + $result = $completioninfo->get_data($cm); + $this->assertTrue(property_exists($result, 'customcompletion')); + + // The data should match when fetching modules individually. + (cache::make('core', 'completion'))->purge(); + foreach ($modinfo->cms as $testcm) { + $result = $completioninfo->get_data($testcm, false); + $this->assertEquals($result, $results[$testcm->id]); + } + } + /** * Tests for completion_info::get_other_cm_completion_data(). * @@ -909,8 +977,15 @@ class completionlib_test extends advanced_testcase { $this->assertEquals($d1, $data->id); $cache = cache::make('core', 'completion'); // Cache was not set for another user. - $this->assertEquals(array('cacherev' => $this->course->cacherev, $cm->id => $data), - $cache->get($data->userid . '_' . $cm->course)); + $cachevalue = $cache->get("{$data->userid}_{$cm->course}"); + $this->assertEquals([ + 'cacherev' => $this->course->cacherev, + $cm->id => array_merge( + (array) $data, + ['other_cm_completion_data_fetched' => true] + ), + ], + $cachevalue); // 2) Test with existing data and for different user. $forum2 = $this->getDataGenerator()->create_module('forum', array('course' => $this->course->id), $completionauto); @@ -928,7 +1003,11 @@ class completionlib_test extends advanced_testcase { $c->internal_set_data($cm2, $d2); // Cache for current user returns the data. $cachevalue = $cache->get($data->userid . '_' . $cm->course); - $this->assertEquals($data, $cachevalue[$cm->id]); + $this->assertEquals(array_merge( + (array) $data, + ['other_cm_completion_data_fetched' => true] + ), $cachevalue[$cm->id]); + // Cache for another user is not filled. $this->assertEquals(false, $cache->get($d2->userid . '_' . $cm2->course));