From 85f47bae7f3e7ded3bce20396b166ff97d500219 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 27 Mar 2019 13:05:35 +0100 Subject: [PATCH] MDL-65204 phpunit: various fixes to assertions Namely: - 3rd param of assertEquals() cannot be null. - Some incorrect uses of assertNotEmpty(). - Comparing 2 strings now uses strict (===) evaluation. Link: https://github.com/sebastianbergmann/phpunit/issues/3185 Solution here is one of: a) Return to the previous situation, making the comparison softer. That can achieved by forcing different types, so float == string works. b) Changing APIs (both forms and database return strings) to perform some conversion to floats. That would make float comparison (with floats or strings) to work too. The patch here follows the a) approach. Changing all the internals for proper float handling sounds excesive when it has been working perfectly since ever. So we went the easier route, just getting rid of the new === comparisons when needed by changing expectation types to float. --- .../dataprivacy/tests/metadata_registry_test.php | 2 +- admin/tool/log/tests/privacy_test.php | 2 +- mod/assign/tests/events_test.php | 4 ++-- mod/assign/tests/externallib_test.php | 6 +++--- mod/assign/tests/locallib_test.php | 8 ++++---- mod/assign/tests/privacy_test.php | 10 +++++----- .../tests/attempt_walkthrough_from_csv_test.php | 2 +- mod/workshop/tests/privacy_provider_test.php | 8 ++++---- question/type/multichoice/tests/helper.php | 16 ++++++++-------- .../multichoice/tests/upgradelibnewqe_test.php | 2 +- 10 files changed, 30 insertions(+), 30 deletions(-) diff --git a/admin/tool/dataprivacy/tests/metadata_registry_test.php b/admin/tool/dataprivacy/tests/metadata_registry_test.php index 2fc88a47a04..143d6b8dd79 100644 --- a/admin/tool/dataprivacy/tests/metadata_registry_test.php +++ b/admin/tool/dataprivacy/tests/metadata_registry_test.php @@ -100,6 +100,6 @@ class tool_dataprivacy_metadata_registry_testcase extends advanced_testcase { $this->assertEquals(1, $corerating['compliant']); $this->assertNotEmpty($corerating['metadata']); $this->assertEquals('database_table', $corerating['metadata'][0]['type']); - $this->assertNotEmpty('database_table', $corerating['metadata'][0]['fields']); + $this->assertNotEmpty($corerating['metadata'][0]['fields']); } } diff --git a/admin/tool/log/tests/privacy_test.php b/admin/tool/log/tests/privacy_test.php index 9e5be714621..ae85b869a65 100644 --- a/admin/tool/log/tests/privacy_test.php +++ b/admin/tool/log/tests/privacy_test.php @@ -66,7 +66,7 @@ class tool_log_privacy_testcase extends provider_testcase { $manager = get_log_manager(true); $this->setUser($u1); - $this->assertEmpty(provider::get_contexts_for_userid($u1->id)->get_contextids(), []); + $this->assertEmpty(provider::get_contexts_for_userid($u1->id)->get_contextids()); $e = \logstore_standard\event\unittest_executed::create(['context' => $c1ctx]); $e->trigger(); $this->assertEquals($c1ctx->id, provider::get_contexts_for_userid($u1->id)->get_contextids()[0]); diff --git a/mod/assign/tests/events_test.php b/mod/assign/tests/events_test.php index fe50250c10d..9b6eb99fdb3 100644 --- a/mod/assign/tests/events_test.php +++ b/mod/assign/tests/events_test.php @@ -629,7 +629,7 @@ class assign_events_testcase extends advanced_testcase { ); $assign->testable_process_save_quick_grades($data); $grade = $assign->get_user_grade($student->id, false); - $this->assertEquals('60.0', $grade->grade); + $this->assertEquals(60.0, $grade->grade); $events = $sink->get_events(); $this->assertCount(3, $events); @@ -655,7 +655,7 @@ class assign_events_testcase extends advanced_testcase { $data->grade = '50.0'; $assign->update_grade($data); $grade = $assign->get_user_grade($student->id, false, 0); - $this->assertEquals('50.0', $grade->grade); + $this->assertEquals(50.0, $grade->grade); $events = $sink->get_events(); $this->assertCount(3, $events); diff --git a/mod/assign/tests/externallib_test.php b/mod/assign/tests/externallib_test.php index e0b3e634b5c..30ebeb5769a 100644 --- a/mod/assign/tests/externallib_test.php +++ b/mod/assign/tests/externallib_test.php @@ -1134,7 +1134,7 @@ class mod_assign_external_testcase extends externallib_advanced_testcase { $result = mod_assign_external::get_grades(array($instance->id)); $result = external_api::clean_returnvalue(mod_assign_external::get_grades_returns(), $result); - $this->assertEquals($result['assignments'][0]['grades'][0]['grade'], '50.0'); + $this->assertEquals((float)$result['assignments'][0]['grades'][0]['grade'], '50.0'); } /** @@ -1284,13 +1284,13 @@ class mod_assign_external_testcase extends externallib_advanced_testcase { array('userid' => $student1->id, 'assignment' => $instance->id), '*', MUST_EXIST); - $this->assertEquals($student1grade->grade, '50.0'); + $this->assertEquals((float)$student1grade->grade, '50.0'); $student2grade = $DB->get_record('assign_grades', array('userid' => $student2->id, 'assignment' => $instance->id), '*', MUST_EXIST); - $this->assertEquals($student2grade->grade, '100.0'); + $this->assertEquals((float)$student2grade->grade, '100.0'); } /** diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 602d0d94247..b9b7dc1966f 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -3640,7 +3640,7 @@ Anchor link 2:Link text $result = $assign->testable_process_save_quick_grades($data); $this->assertContains(get_string('quickgradingchangessaved', 'assign'), $result); $grade = $assign->get_user_grade($student->id, false); - $this->assertEquals('60.0', $grade->grade); + $this->assertEquals(60.0, $grade->grade); // Attempt to grade with a past attempts grade info. $assign->testable_process_add_attempt($student->id); @@ -3664,7 +3664,7 @@ Anchor link 2:Link text $result = $assign->testable_process_save_quick_grades($data); $this->assertContains(get_string('quickgradingchangessaved', 'assign'), $result); $grade = $assign->get_user_grade($student->id, false); - $this->assertEquals('40.0', $grade->grade); + $this->assertEquals(40.0, $grade->grade); // Catch grade update conflicts. // Save old data for later. @@ -3678,13 +3678,13 @@ Anchor link 2:Link text $result = $assign->testable_process_save_quick_grades($data); $this->assertContains(get_string('quickgradingchangessaved', 'assign'), $result); $grade = $assign->get_user_grade($student->id, false); - $this->assertEquals('30.0', $grade->grade); + $this->assertEquals(30.0, $grade->grade); // Now update using 'old' data. Should fail. $result = $assign->testable_process_save_quick_grades($pastdata); $this->assertContains(get_string('errorrecordmodified', 'assign'), $result); $grade = $assign->get_user_grade($student->id, false); - $this->assertEquals('30.0', $grade->grade); + $this->assertEquals(30.0, $grade->grade); } /** diff --git a/mod/assign/tests/privacy_test.php b/mod/assign/tests/privacy_test.php index 185d5aaf8e6..fe2f9aa1f64 100644 --- a/mod/assign/tests/privacy_test.php +++ b/mod/assign/tests/privacy_test.php @@ -315,8 +315,8 @@ class mod_assign_privacy_testcase extends provider_testcase { $this->assertEquals(1, $writer->get_data(['attempt 1', 'submission'])->attemptnumber); $this->assertEquals(2, $writer->get_data(['attempt 2', 'submission'])->attemptnumber); // Check grades. - $this->assertEquals($grade1, $writer->get_data(['attempt 1', 'grade'])->grade); - $this->assertEquals($grade2, $writer->get_data(['attempt 2', 'grade'])->grade); + $this->assertEquals((float)$grade1, $writer->get_data(['attempt 1', 'grade'])->grade); + $this->assertEquals((float)$grade2, $writer->get_data(['attempt 2', 'grade'])->grade); // Check feedback. $this->assertContains($teachercommenttext, $writer->get_data(['attempt 1', 'Feedback comments'])->commenttext); $this->assertContains($teachercommenttext2, $writer->get_data(['attempt 2', 'Feedback comments'])->commenttext); @@ -425,11 +425,11 @@ class mod_assign_privacy_testcase extends provider_testcase { // Check for student grades given. $student1grade = $writer->get_data(['studentsubmissions', $user1->id, 'attempt 1', 'grade']); - $this->assertEquals($grade1, $student1grade->grade); + $this->assertEquals((float)$grade1, $student1grade->grade); $student2grade1 = $writer->get_data(['studentsubmissions', $user2->id, 'attempt 1', 'grade']); - $this->assertEquals($grade2, $student2grade1->grade); + $this->assertEquals((float)$grade2, $student2grade1->grade); $student2grade2 = $writer->get_data(['studentsubmissions', $user2->id, 'attempt 2', 'grade']); - $this->assertEquals($grade3, $student2grade2->grade); + $this->assertEquals((float)$grade3, $student2grade2->grade); // Check for feedback given to students. $this->assertContains($teachercommenttext, $writer->get_data(['studentsubmissions', $user1->id, 'attempt 1', 'Feedback comments'])->commenttext); diff --git a/mod/quiz/tests/attempt_walkthrough_from_csv_test.php b/mod/quiz/tests/attempt_walkthrough_from_csv_test.php index 3556793f612..029dbd2ffb1 100644 --- a/mod/quiz/tests/attempt_walkthrough_from_csv_test.php +++ b/mod/quiz/tests/attempt_walkthrough_from_csv_test.php @@ -338,7 +338,7 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { $this->assertEquals((bool)$value, $attemptobj->is_finished()); break; case 'summarks' : - $this->assertEquals($value, $attemptobj->get_sum_marks(), "Sum of marks of attempt {$result['quizattempt']}."); + $this->assertEquals((float)$value, $attemptobj->get_sum_marks(), "Sum of marks of attempt {$result['quizattempt']}."); break; case 'quizgrade' : // Check quiz grades. diff --git a/mod/workshop/tests/privacy_provider_test.php b/mod/workshop/tests/privacy_provider_test.php index e0c6d81f516..a59388deea8 100644 --- a/mod/workshop/tests/privacy_provider_test.php +++ b/mod/workshop/tests/privacy_provider_test.php @@ -189,19 +189,19 @@ class mod_workshop_privacy_provider_testcase extends advanced_testcase { // Student1 has data in workshop11 (author + self reviewer), workshop12 (author) and workshop21 (reviewer). $contextlist = \mod_workshop\privacy\provider::get_contexts_for_userid($this->student1->id); $this->assertInstanceOf(\core_privacy\local\request\contextlist::class, $contextlist); - $this->assertEquals([$context11->id, $context12->id, $context21->id], $contextlist->get_contextids(), null, 0.0, 10, true); + $this->assertEquals([$context11->id, $context12->id, $context21->id], $contextlist->get_contextids(), '', 0.0, 10, true); // Student2 has data in workshop11 (reviewer), workshop12 (reviewer) and workshop21 (author). $contextlist = \mod_workshop\privacy\provider::get_contexts_for_userid($this->student2->id); - $this->assertEquals([$context11->id, $context12->id, $context21->id], $contextlist->get_contextids(), null, 0.0, 10, true); + $this->assertEquals([$context11->id, $context12->id, $context21->id], $contextlist->get_contextids(), '', 0.0, 10, true); // Student3 has data in workshop11 (reviewer). $contextlist = \mod_workshop\privacy\provider::get_contexts_for_userid($this->student3->id); - $this->assertEquals([$context11->id], $contextlist->get_contextids(), null, 0.0, 10, true); + $this->assertEquals([$context11->id], $contextlist->get_contextids(), '', 0.0, 10, true); // Teacher4 has data in workshop12 (gradeoverby) and workshop21 (gradinggradeoverby). $contextlist = \mod_workshop\privacy\provider::get_contexts_for_userid($this->teacher4->id); - $this->assertEquals([$context21->id, $context12->id], $contextlist->get_contextids(), null, 0.0, 10, true); + $this->assertEquals([$context21->id, $context12->id], $contextlist->get_contextids(), '', 0.0, 10, true); } /** diff --git a/question/type/multichoice/tests/helper.php b/question/type/multichoice/tests/helper.php index 27297541b5e..b665052ccb6 100644 --- a/question/type/multichoice/tests/helper.php +++ b/question/type/multichoice/tests/helper.php @@ -80,7 +80,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 13, 'answer' => 'One', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '0.5', + 'fraction' => 0.5, 'feedback' => 'One is odd.', 'feedbackformat' => FORMAT_HTML, ), @@ -88,7 +88,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 14, 'answer' => 'Two', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '0.0', + 'fraction' => 0.0, 'feedback' => 'Two is even.', 'feedbackformat' => FORMAT_HTML, ), @@ -96,7 +96,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 15, 'answer' => 'Three', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '0.5', + 'fraction' => 0.5, 'feedback' => 'Three is odd.', 'feedbackformat' => FORMAT_HTML, ), @@ -104,7 +104,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 16, 'answer' => 'Four', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '0.0', + 'fraction' => 0.0, 'feedback' => 'Four is even.', 'feedbackformat' => FORMAT_HTML, ), @@ -261,7 +261,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 13, 'answer' => 'One', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '1', + 'fraction' => 1, 'feedback' => 'One is the oddest.', 'feedbackformat' => FORMAT_HTML, ), @@ -269,7 +269,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 14, 'answer' => 'Two', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '0.0', + 'fraction' => 0.0, 'feedback' => 'Two is even.', 'feedbackformat' => FORMAT_HTML, ), @@ -277,7 +277,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 15, 'answer' => 'Three', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '0', + 'fraction' => 0, 'feedback' => 'Three is odd.', 'feedbackformat' => FORMAT_HTML, ), @@ -285,7 +285,7 @@ class qtype_multichoice_test_helper extends question_test_helper { 'id' => 16, 'answer' => 'Four', 'answerformat' => FORMAT_PLAIN, - 'fraction' => '0.0', + 'fraction' => 0.0, 'feedback' => 'Four is even.', 'feedbackformat' => FORMAT_HTML, ), diff --git a/question/type/multichoice/tests/upgradelibnewqe_test.php b/question/type/multichoice/tests/upgradelibnewqe_test.php index f6859b72c41..cd94e1a4cd4 100644 --- a/question/type/multichoice/tests/upgradelibnewqe_test.php +++ b/question/type/multichoice/tests/upgradelibnewqe_test.php @@ -266,7 +266,7 @@ class qtype_multichoice_attempt_upgrader_test extends question_attempt_upgrader_ 'fraction' => 0.9, 'timecreated' => 1278604597, 'userid' => null, - 'data' => array('-comment' => 'Well done!', '-mark' => '0.9', '-maxmark' => '1'), + 'data' => array('-comment' => 'Well done!', '-mark' => '0.9', '-maxmark' => 1), ), ), ); -- 2.43.0