diff --git a/lib/phpunit/classes/advanced_testcase.php b/lib/phpunit/classes/advanced_testcase.php index 87d2f442c7c..9674a480156 100644 --- a/lib/phpunit/classes/advanced_testcase.php +++ b/lib/phpunit/classes/advanced_testcase.php @@ -390,7 +390,8 @@ abstract class advanced_testcase extends base_testcase { } /** - * Assert that an event is not using event->contxet. + * Assert that various event methods are not using event->context + * * While restoring context might not be valid and it should not be used by event url * or description methods. * @@ -410,7 +411,7 @@ abstract class advanced_testcase extends base_testcase { $event->get_url(); $event->get_description(); - // Restore event->context. + // Restore event->context (note that this is unreachable when the event uses context). But ok for correct events. phpunit_event_mock::testable_set_event_context($event, $eventcontext); } diff --git a/lib/phpunit/tests/advanced_test.php b/lib/phpunit/tests/advanced_test.php index 2088a4e7baa..6aa4ba18ccb 100644 --- a/lib/phpunit/tests/advanced_test.php +++ b/lib/phpunit/tests/advanced_test.php @@ -339,6 +339,54 @@ class advanced_test extends \advanced_testcase { } } + /** + * Test the assertEventContextNotUsed() assertion. + * + * Verify that events using the event context in some of their + * methods are detected properly (will throw a warning if they are). + * + * To do so, we'll be using some fixture events (context_used_in_event_xxxx), + * that, on purpose, use the event context (incorrectly) in their methods. + * + * Note that because we are using imported fixture classes, and because we + * are testing for warnings, better we run the tests in a separate process. + * + * @param string $fixture The fixture class to use. + * @param bool $phpwarn Whether a PHP warning is expected. + * + * @runInSeparateProcess + * @dataProvider assert_event_context_not_used_provider + * @covers ::assertEventContextNotUsed + */ + public function test_assert_event_context_not_used($fixture, $phpwarn): void { + require(__DIR__ . '/fixtures/event_fixtures.php'); + // Create an event that uses the event context in its get_url() and get_description() methods. + $event = $fixture::create([ + 'other' => [ + 'sample' => 1, + 'xx' => 10, + ], + ]); + + if ($phpwarn) { + $this->expectWarning(); + } + $this->assertEventContextNotUsed($event); + } + + /** + * Data provider for test_assert_event_context_not_used(). + * + * @return array + */ + public static function assert_event_context_not_used_provider(): array { + return [ + 'correct' => ['\core\event\context_used_in_event_correct', false], + 'wrong_get_url' => ['\core\event\context_used_in_event_get_url', true], + 'wrong_get_description' => ['\core\event\context_used_in_event_get_description', true], + ]; + } + public function test_message_processors_reset() { global $DB; diff --git a/lib/phpunit/tests/fixtures/event_fixtures.php b/lib/phpunit/tests/fixtures/event_fixtures.php new file mode 100644 index 00000000000..7dfa6118211 --- /dev/null +++ b/lib/phpunit/tests/fixtures/event_fixtures.php @@ -0,0 +1,80 @@ +. + +/** + * Fixtures for advanced_testcase tests. + * + * @package core + * @category event + * @copyright 2024 onwards Eloy Lafuente (stronk7) {@link https://stronk7.com} + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core\event; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Event to test that \advanced_testcase::assertEventContextNotUsed() passes ok when no context is used. + */ +class context_used_in_event_correct extends \core\event\base { + + protected function init() { + $this->data['crud'] = 'u'; + $this->data['edulevel'] = self::LEVEL_PARTICIPATING; + $this->context = \context_system::instance(); + } + + public function get_url() { + return new \moodle_url('/somepath/somefile.php'); // No context used. + } + + public function get_description() { + return 'Description'; // No context used. + } +} + +/** + * Event to test that \advanced_testcase::assertEventContextNotUsed() detects context usage on get_url(). + */ +class context_used_in_event_get_url extends \core\event\base { + + protected function init() { + $this->data['crud'] = 'u'; + $this->data['edulevel'] = self::LEVEL_PARTICIPATING; + $this->context = \context_system::instance(); + } + + public function get_url() { + return new \moodle_url('/somepath/somefile.php', ['id' => $this->context->instanceid]); // Causes a PHP Warning. + } +} + +/** + * Event to test that \advanced_testcase::assertEventContextNotUsed() detects context usage on get_description(). + */ +class context_used_in_event_get_description extends \core\event\base { + + protected function init() { + $this->data['crud'] = 'u'; + $this->data['edulevel'] = self::LEVEL_PARTICIPATING; + $this->context = \context_system::instance(); + } + + public function get_description() { + return $this->context->instanceid . " Description"; // Causes a PHP Warning. + } +} diff --git a/lib/tests/event/base_test.php b/lib/tests/event/base_test.php index d749c543d2a..30dc2c91cbe 100644 --- a/lib/tests/event/base_test.php +++ b/lib/tests/event/base_test.php @@ -853,27 +853,6 @@ class base_test extends \advanced_testcase { $this->assertSame($event->get_data(), $data); } - public function test_context_not_used() { - // TODO: MDL-69688 - This test is far away from my understanding. It throws a - // "Trying to get property 'instanceid' of non-object" notice, so - // it's not clear for me what the test is doing. This was detected - // when preparing tests for PHPUnit 8 (MDL-67673) and, at the end - // all that was done is to move the annotation (deprecated) to - // explicit expectation. Still try commenting it out and you'll see - // the notice. - if (PHP_VERSION_ID >= 80000) { - $this->expectWarning(); - } else { - $this->expectNotice(); - } - $event = \core_tests\event\context_used_in_event::create(array('other' => array('sample' => 1, 'xx' => 10))); - $this->assertEventContextNotUsed($event); - - $eventcontext = phpunit_event_mock::testable_get_event_context($event); - phpunit_event_mock::testable_set_event_context($event, null); - $this->assertEventContextNotUsed($event); - } - /** * Test that all observer information is returned correctly. */ diff --git a/lib/tests/fixtures/event_fixtures.php b/lib/tests/fixtures/event_fixtures.php index 33d02ceb2ff..a8c3bfd5a0f 100644 --- a/lib/tests/fixtures/event_fixtures.php +++ b/lib/tests/fixtures/event_fixtures.php @@ -244,25 +244,6 @@ class course_module_viewed_noinit extends \core\event\course_module_viewed { class grade_report_viewed extends \core\event\grade_report_viewed { } -/** - * Event to test context used in event functions - */ -class context_used_in_event extends \core\event\base { - public function get_description() { - return $this->context->instanceid . " Description"; - } - - protected function init() { - $this->data['crud'] = 'u'; - $this->data['edulevel'] = self::LEVEL_PARTICIPATING; - $this->context = \context_system::instance(); - } - - public function get_url() { - return new \moodle_url('/somepath/somefile.php', array('id' => $this->context->instanceid)); - } -} - /** * This is an explanation of the event. * - I'm making a point here.