From 17237b824da353ba47aa2028b64c1fb986d32eda Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 17 Apr 2024 16:17:27 +0200 Subject: [PATCH 1/3] MDL-81581 phpunit: Move multiple assert_same_xml() to common one --- question/engine/tests/helpers.php | 13 +++++++++++++ question/format/xml/tests/xmlformat_test.php | 5 ----- question/type/ddwtos/tests/question_type_test.php | 5 ----- .../type/gapselect/tests/question_type_test.php | 11 ----------- question/type/ordering/tests/questiontype_test.php | 11 ----------- 5 files changed, 13 insertions(+), 32 deletions(-) diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index f83d9697a41..1e4ec1a38d1 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -602,6 +602,19 @@ abstract class question_testcase extends advanced_testcase { } return; } + + /** + * Check that 2 XML strings are the same, ignoring differences in line endings. + * + * @param string $expectedxml The expected XML string + * @param string $xml The XML string to check + */ + public function assert_same_xml($expectedxml, $xml) { + $this->assertEquals( + str_replace("\r\n", "\n", $expectedxml), + str_replace("\r\n", "\n", $xml) + ); + } } diff --git a/question/format/xml/tests/xmlformat_test.php b/question/format/xml/tests/xmlformat_test.php index fd8138819c7..7362a1b2295 100644 --- a/question/format/xml/tests/xmlformat_test.php +++ b/question/format/xml/tests/xmlformat_test.php @@ -47,11 +47,6 @@ require_once($CFG->dirroot . '/question/engine/tests/helpers.php'); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class xmlformat_test extends \question_testcase { - public function assert_same_xml($expectedxml, $xml) { - $this->assertEquals(str_replace("\r\n", "\n", $expectedxml), - str_replace("\r\n", "\n", $xml)); - } - public function make_test_question() { global $USER; $q = new \stdClass(); diff --git a/question/type/ddwtos/tests/question_type_test.php b/question/type/ddwtos/tests/question_type_test.php index 16de9a09e35..347bc169260 100644 --- a/question/type/ddwtos/tests/question_type_test.php +++ b/question/type/ddwtos/tests/question_type_test.php @@ -47,11 +47,6 @@ class question_type_test extends \question_testcase { $this->qtype = null; } - public function assert_same_xml($expectedxml, $xml) { - $this->assertEquals(str_replace("\r\n", "\n", $expectedxml), - str_replace("\r\n", "\n", $xml)); - } - /** * Get some test question data. * diff --git a/question/type/gapselect/tests/question_type_test.php b/question/type/gapselect/tests/question_type_test.php index 019542fc56f..0ad8b3b40af 100644 --- a/question/type/gapselect/tests/question_type_test.php +++ b/question/type/gapselect/tests/question_type_test.php @@ -46,17 +46,6 @@ class question_type_test extends \question_testcase { $this->qtype = null; } - /** - * Asserts that two strings containing XML are the same ignoring the line-endings. - * - * @param string $expectedxml - * @param string $xml - */ - public function assert_same_xml($expectedxml, $xml) { - $this->assertEquals(str_replace("\r\n", "\n", $expectedxml), - str_replace("\r\n", "\n", $xml)); - } - public function test_save_question() { $this->resetAfterTest(); diff --git a/question/type/ordering/tests/questiontype_test.php b/question/type/ordering/tests/questiontype_test.php index e9b98b3795f..c82f9ee8066 100644 --- a/question/type/ordering/tests/questiontype_test.php +++ b/question/type/ordering/tests/questiontype_test.php @@ -72,17 +72,6 @@ final class questiontype_test extends \question_testcase { ]; } - /** - * Asserts that two XML strings are the same, ignoring differences in line endings. - * - * @param string $expectedxml - * @param string $xml - */ - public function assert_same_xml(string $expectedxml, string $xml): void { - $this->assertEquals(str_replace("\r\n", "\n", $expectedxml), - str_replace("\r\n", "\n", $xml)); - } - public function test_name(): void { $ordering = new qtype_ordering(); $this->assertEquals('ordering', $ordering->name()); From 71bd82ad6877783e8cf1dcddad6ac817b96b95cd Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 17 Apr 2024 16:25:45 +0200 Subject: [PATCH 2/3] MDL-81581 phpunit: Create the normalise_line_endings() method And apply it to all the obvious places related with the issue. Note that surely there are way more in code base, but it's out of scope for this issue. --- lib/phpunit/classes/util.php | 10 ++++++++++ question/engine/tests/helpers.php | 4 ++-- .../xml/tests/qformat_xml_import_export_test.php | 2 +- question/type/ordering/tests/questiontype_test.php | 6 +++++- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index 22454da39f5..0e0bb2d1cb6 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -990,6 +990,16 @@ class phpunit_util extends testing_util { return str_repeat(" ", $level * 2) . "{$string}\n"; } + /** + * Normalise any text to always use unix line endings (line-feeds). + * + * @param string $text The text to normalize + * @return string + */ + public static function normalise_line_endings(string $text): string { + return str_replace(["\r\n", "\r"], "\n", $text); + } + /** * Get the coverage config for the supplied includelist and excludelist configuration. * diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index 1e4ec1a38d1..407374e8562 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -611,8 +611,8 @@ abstract class question_testcase extends advanced_testcase { */ public function assert_same_xml($expectedxml, $xml) { $this->assertEquals( - str_replace("\r\n", "\n", $expectedxml), - str_replace("\r\n", "\n", $xml) + phpunit_util::normalise_line_endings($expectedxml), + phpunit_util::normalise_line_endings($xml) ); } } diff --git a/question/format/xml/tests/qformat_xml_import_export_test.php b/question/format/xml/tests/qformat_xml_import_export_test.php index 975770ff4e6..11ec1a6a528 100644 --- a/question/format/xml/tests/qformat_xml_import_export_test.php +++ b/question/format/xml/tests/qformat_xml_import_export_test.php @@ -78,7 +78,7 @@ class qformat_xml_import_export_test extends advanced_testcase { */ protected function normalise_xml($xml) { // Normalise line endings. - $xml = str_replace("\r\n", "\n", $xml); + $xml = phpunit_util::normalise_line_endings($xml); $xml = preg_replace("~\n$~", "", $xml); // Strip final newline in file. // Replace all numbers in question id comments with 0. diff --git a/question/type/ordering/tests/questiontype_test.php b/question/type/ordering/tests/questiontype_test.php index c82f9ee8066..00981c4fe45 100644 --- a/question/type/ordering/tests/questiontype_test.php +++ b/question/type/ordering/tests/questiontype_test.php @@ -17,6 +17,7 @@ namespace qtype_ordering; use core_question_generator; +use phpunit_util; use qtype_ordering; use qtype_ordering_test_helper; use qtype_ordering_edit_form; @@ -315,6 +316,9 @@ final class questiontype_test extends \question_testcase { $expectedgift = file_get_contents(__DIR__ . '/fixtures/testexport.gift.txt'); - $this->assertEquals($expectedgift, $gift); + $this->assertEquals( + phpunit_util::normalise_line_endings($expectedgift), + phpunit_util::normalise_line_endings($gift) + ); } } From dadfb6e90cf6f5e2d8bfede8ef174ed9502fa4ac Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 24 Apr 2024 12:00:54 +0200 Subject: [PATCH 3/3] MDL-81581 phpunit: Apply the new method to all remaining cases After discussing it in the issue, we have agreed to apply the new phpunit_util::normalise_line_endings() method to all the remaining cases (within tests!) where the manual replacement was being done. This commits achieves that. Note that I've looked for both str_replace() and preg_replace() cases, but only the former had cases worth converting. All the later ones are different and cannot be replaced by the new utility method. --- backup/util/xml/tests/writer_test.php | 3 +- .../aiken/tests/qformat_aiken_export_test.php | 6 ++-- .../format/gift/tests/giftformat_test.php | 35 ++++++++++--------- .../type/ordering/tests/questiontype_test.php | 2 +- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/backup/util/xml/tests/writer_test.php b/backup/util/xml/tests/writer_test.php index 2e5c4543551..109d26ba005 100644 --- a/backup/util/xml/tests/writer_test.php +++ b/backup/util/xml/tests/writer_test.php @@ -26,6 +26,7 @@ namespace core_backup; use memory_xml_output; +use phpunit_util; use xml_contenttransformer; use xml_output; use xml_writer; @@ -314,7 +315,7 @@ class writer_test extends \basic_testcase { $fcontents = file_get_contents($CFG->dirroot . '/backup/util/xml/tests/fixtures/test1.xml'); // Normalise carriage return characters. - $fcontents = str_replace("\r\n", "\n", $fcontents); + $fcontents = phpunit_util::normalise_line_endings($fcontents); $this->assertEquals(trim($result), trim($fcontents)); } } diff --git a/question/format/aiken/tests/qformat_aiken_export_test.php b/question/format/aiken/tests/qformat_aiken_export_test.php index ac0eb13d841..4623e521106 100644 --- a/question/format/aiken/tests/qformat_aiken_export_test.php +++ b/question/format/aiken/tests/qformat_aiken_export_test.php @@ -44,8 +44,10 @@ class qformat_aiken_export_test extends advanced_testcase { * @param string $text The actual string. */ public function assert_same_aiken($expectedtext, $text) { - $this->assertEquals(str_replace("\r\n", "\n", $expectedtext), - str_replace("\r\n", "\n", $text)); + $this->assertEquals( + phpunit_util::normalise_line_endings($expectedtext), + phpunit_util::normalise_line_endings($text) + ); } public function test_export_questions() { diff --git a/question/format/gift/tests/giftformat_test.php b/question/format/gift/tests/giftformat_test.php index ef0321b2ee7..1366e8cc073 100644 --- a/question/format/gift/tests/giftformat_test.php +++ b/question/format/gift/tests/giftformat_test.php @@ -16,6 +16,7 @@ namespace qformat_gift; +use phpunit_util; use qformat_gift; use question_bank; use question_check_specified_fields_expectation; @@ -37,15 +38,17 @@ require_once($CFG->dirroot . '/question/engine/tests/helpers.php'); */ class giftformat_test extends \question_testcase { public function assert_same_gift($expectedtext, $text) { - $this->assertEquals(str_replace("\r\n", "\n", $expectedtext), - str_replace("\r\n", "\n", $text)); + $this->assertEquals( + phpunit_util::normalise_line_endings($expectedtext), + phpunit_util::normalise_line_endings($text) + ); } public function test_import_essay() { $gift = ' // essay ::Q8:: How are you? {}'; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -114,7 +117,7 @@ class giftformat_test extends \question_testcase { =[markdown]A collection of web pages that anyone can add to or edit. -> Wiki = -> Chat }'; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -284,7 +287,7 @@ class giftformat_test extends \question_testcase { ~red # [html]wrong, it's yellow ~[plain]blue # wrong, it's yellow }"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -387,7 +390,7 @@ class giftformat_test extends \question_testcase { ~%50%off-beige # right; good! ~%-100%[plain]blue # wrong }"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -482,7 +485,7 @@ class giftformat_test extends \question_testcase { ~%-50%red # wrong ~%-50%blue # wrong }"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -694,7 +697,7 @@ class giftformat_test extends \question_testcase { $gift = " // math range question ::Q5:: What is a number from 1 to 5? {#3:2~#Completely wrong}"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -800,7 +803,7 @@ class giftformat_test extends \question_testcase { =%50%Cat#What is it with Moodlers and cats? =%0%*#Completely wrong }"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -856,7 +859,7 @@ class giftformat_test extends \question_testcase { =%0%*#Completely wrong ####[html]Here is some general feedback! }"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -1029,7 +1032,7 @@ class giftformat_test extends \question_testcase { // true/false ::Q1:: 42 is the Absolute Answer to everything.{ FALSE#42 is the Ultimate Answer.#You gave the right answer.}"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -1063,7 +1066,7 @@ FALSE#42 is the Ultimate Answer.#You gave the right answer.}"; public function test_import_truefalse_true_answer1() { $gift = "// name 0-11 ::2-08 TSL::TSL is blablabla.{T}"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -1097,7 +1100,7 @@ FALSE#42 is the Ultimate Answer.#You gave the right answer.}"; public function test_import_truefalse_true_answer2() { $gift = "// name 0-11 ::2-08 TSL::TSL is blablabla.{TRUE}"; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -1219,7 +1222,7 @@ FALSE#42 is the Ultimate Answer.#You gave the right answer.}"; $gift = ' // essay ::double backslash:: A \\\\ B \\\\\\\\ C{}'; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -1257,7 +1260,7 @@ FALSE#42 is the Ultimate Answer.#You gave the right answer.}"; \} {}'; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); @@ -1296,7 +1299,7 @@ FALSE#42 is the Ultimate Answer.#You gave the right answer.}"; // This question is to test importing tags: [tag:tag] [tag:other-tag]. // And an idnumber: [id:myid]. ::Question name:: How are you? {}'; - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $importer = new qformat_gift(); $q = $importer->readquestion($lines); diff --git a/question/type/ordering/tests/questiontype_test.php b/question/type/ordering/tests/questiontype_test.php index 00981c4fe45..5a390258ba4 100644 --- a/question/type/ordering/tests/questiontype_test.php +++ b/question/type/ordering/tests/questiontype_test.php @@ -293,7 +293,7 @@ final class questiontype_test extends \question_testcase { // Import a question from GIFT. $gift = file_get_contents(__DIR__ . '/fixtures/testimport.gift.txt'); $format = new qformat_gift(); - $lines = preg_split('/[\\n\\r]/', str_replace("\r\n", "\n", $gift)); + $lines = preg_split('/[\\n\\r]/', phpunit_util::normalise_line_endings($gift)); $imported = $format->readquestion($lines); $this->assert(new question_check_specified_fields_expectation(self::expectedimport()), $imported);