From b060e7495c11e07bec4960bb342f9f9edcd7b2dd Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 11 Mar 2020 11:01:56 +0000 Subject: [PATCH 1/2] MDL-68149 qtype_match: Updatey tests to follow make_question() pattern --- .../tests/walkthrough_test.php | 2 +- question/engine/tests/helpers.php | 24 +------------- question/type/match/question.php | 2 ++ question/type/match/tests/helper.php | 33 ++++++++++++++++++- question/type/match/tests/question_test.php | 28 ++++++++-------- .../type/match/tests/walkthrough_test.php | 14 ++++---- 6 files changed, 57 insertions(+), 46 deletions(-) diff --git a/question/behaviour/interactivecountback/tests/walkthrough_test.php b/question/behaviour/interactivecountback/tests/walkthrough_test.php index 583a41bce33..255cbe71eff 100644 --- a/question/behaviour/interactivecountback/tests/walkthrough_test.php +++ b/question/behaviour/interactivecountback/tests/walkthrough_test.php @@ -42,7 +42,7 @@ class qbehaviour_interactivecountback_walkthrough_test extends qbehaviour_walkth public function test_interactive_feedback_match_reset() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->shufflestems = false; $m->hints = array( new question_hint_with_parts(0, 'This is the first hint.', FORMAT_HTML, true, true), diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index d602098b828..32cd480597a 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -375,29 +375,7 @@ class test_question_maker { * @return qtype_match_question */ public static function make_a_matching_question() { - question_bank::load_question_definition_classes('match'); - $match = new qtype_match_question(); - self::initialise_a_question($match); - $match->name = 'Matching question'; - $match->questiontext = 'Classify the animals.'; - $match->generalfeedback = 'Frogs and toads are amphibians, the others are mammals.'; - $match->qtype = question_bank::get_qtype('match'); - - $match->shufflestems = 1; - - self::set_standard_combined_feedback_fields($match); - - // Using unset to get 1-based arrays. - $match->stems = array('', 'Dog', 'Frog', 'Toad', 'Cat'); - $match->stemformat = array('', FORMAT_HTML, FORMAT_HTML, FORMAT_HTML, FORMAT_HTML); - $match->choices = array('', 'Mammal', 'Amphibian', 'Insect'); - $match->right = array('', 1, 2, 2, 1); - unset($match->stems[0]); - unset($match->stemformat[0]); - unset($match->choices[0]); - unset($match->right[0]); - - return $match; + return self::make_question('match'); } /** diff --git a/question/type/match/question.php b/question/type/match/question.php index 41130100daf..6aba21d82fa 100644 --- a/question/type/match/question.php +++ b/question/type/match/question.php @@ -46,6 +46,8 @@ class qtype_match_question extends question_graded_automatically_with_countback /** @var array of question stems. */ public $stems; + /** @var int[] FORMAT_... type for each stem. */ + public $stemformat; /** @var array of choices that can be matched to each stem. */ public $choices; /** @var array index of the right choice for each stem. */ diff --git a/question/type/match/tests/helper.php b/question/type/match/tests/helper.php index 1e0aabbddd4..ee5ab4dd909 100644 --- a/question/type/match/tests/helper.php +++ b/question/type/match/tests/helper.php @@ -40,7 +40,6 @@ class qtype_match_test_helper extends question_test_helper { return array('foursubq'); } - /** * Makes a match question about completing two blanks in some text. * @return object the question definition data, as it might be returned from @@ -128,4 +127,36 @@ class qtype_match_test_helper extends question_test_helper { return $q; } + /** + * Makes a matching question to classify 'Dog', 'Frog', 'Toad' and 'Cat' as + * 'Mammal', 'Amphibian' or 'Insect'. + * defaultmark 1. Stems are shuffled by default. + * @return qtype_match_question + */ + public static function make_match_question_foursubq() { + question_bank::load_question_definition_classes('match'); + $match = new qtype_match_question(); + test_question_maker::initialise_a_question($match); + $match->name = 'Matching question'; + $match->questiontext = 'Classify the animals.'; + $match->generalfeedback = 'Frogs and toads are amphibians, the others are mammals.'; + $match->qtype = question_bank::get_qtype('match'); + + $match->shufflestems = 1; + + test_question_maker::set_standard_combined_feedback_fields($match); + + // Using unset to get 1-based arrays. + $match->stems = array('', 'Dog', 'Frog', 'Toad', 'Cat'); + $match->stemformat = array('', FORMAT_HTML, FORMAT_HTML, FORMAT_HTML, FORMAT_HTML); + $match->choices = array('', 'Mammal', 'Amphibian', 'Insect'); + $match->right = array('', 1, 2, 2, 1); + unset($match->stems[0]); + unset($match->stemformat[0]); + unset($match->choices[0]); + unset($match->right[0]); + + return $match; + } + } diff --git a/question/type/match/tests/question_test.php b/question/type/match/tests/question_test.php index 4e83431d2c3..67ab4012aa3 100644 --- a/question/type/match/tests/question_test.php +++ b/question/type/match/tests/question_test.php @@ -38,7 +38,7 @@ require_once($CFG->dirroot . '/question/engine/tests/helpers.php'); class qtype_match_question_test extends advanced_testcase { public function test_get_expected_data() { - $question = test_question_maker::make_a_matching_question(); + $question = test_question_maker::make_question('match'); $question->start_attempt(new question_attempt_step(), 1); $this->assertEquals(array('sub0' => PARAM_INT, 'sub1' => PARAM_INT, @@ -46,7 +46,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_is_complete_response() { - $question = test_question_maker::make_a_matching_question(); + $question = test_question_maker::make_question('match'); $question->start_attempt(new question_attempt_step(), 1); $this->assertFalse($question->is_complete_response(array())); @@ -58,7 +58,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_is_gradable_response() { - $question = test_question_maker::make_a_matching_question(); + $question = test_question_maker::make_question('match'); $question->start_attempt(new question_attempt_step(), 1); $this->assertFalse($question->is_gradable_response(array())); @@ -72,7 +72,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_is_same_response() { - $question = test_question_maker::make_a_matching_question(); + $question = test_question_maker::make_question('match'); $question->start_attempt(new question_attempt_step(), 1); $this->assertTrue($question->is_same_response( @@ -97,7 +97,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_grading() { - $question = test_question_maker::make_a_matching_question(); + $question = test_question_maker::make_question('match'); $question->start_attempt(new question_attempt_step(), 1); $correctresponse = $question->prepare_simulated_post_data( @@ -126,7 +126,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_get_correct_response() { - $question = test_question_maker::make_a_matching_question(); + $question = test_question_maker::make_question('match'); $question->start_attempt(new question_attempt_step(), 1); $correct = $question->prepare_simulated_post_data(array('Dog' => 'Mammal', @@ -137,7 +137,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_get_question_summary() { - $match = test_question_maker::make_a_matching_question(); + $match = test_question_maker::make_question('match'); $match->start_attempt(new question_attempt_step(), 1); $qsummary = $match->get_question_summary(); $this->assertRegExp('/' . preg_quote($match->questiontext, '/') . '/', $qsummary); @@ -150,7 +150,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_summarise_response() { - $match = test_question_maker::make_a_matching_question(); + $match = test_question_maker::make_question('match'); $match->start_attempt(new question_attempt_step(), 1); $summary = $match->summarise_response($match->prepare_simulated_post_data(array('Dog' => 'Amphibian', 'Frog' => 'Mammal'))); @@ -160,7 +160,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_classify_response() { - $match = test_question_maker::make_a_matching_question(); + $match = test_question_maker::make_question('match'); $match->start_attempt(new question_attempt_step(), 1); $response = $match->prepare_simulated_post_data(array('Dog' => 'Amphibian', 'Frog' => 'Insect', 'Toad' => '', 'Cat' => '')); @@ -182,14 +182,14 @@ class qtype_match_question_test extends advanced_testcase { } public function test_classify_response_choice_deleted_after_attempt() { - $match = test_question_maker::make_a_matching_question(); + $match = test_question_maker::make_question('match'); $firststep = new question_attempt_step(); $match->start_attempt($firststep, 1); $response = $match->prepare_simulated_post_data(array( 'Dog' => 'Amphibian', 'Frog' => 'Insect', 'Toad' => '', 'Cat' => 'Mammal')); - $match = test_question_maker::make_a_matching_question(); + $match = test_question_maker::make_question('match'); unset($match->stems[4]); unset($match->stemsformat[4]); unset($match->right[4]); @@ -203,14 +203,14 @@ class qtype_match_question_test extends advanced_testcase { } public function test_classify_response_choice_added_after_attempt() { - $match = test_question_maker::make_a_matching_question(); + $match = test_question_maker::make_question('match'); $firststep = new question_attempt_step(); $match->start_attempt($firststep, 1); $response = $match->prepare_simulated_post_data(array( 'Dog' => 'Amphibian', 'Frog' => 'Insect', 'Toad' => '', 'Cat' => 'Mammal')); - $match = test_question_maker::make_a_matching_question(); + $match = test_question_maker::make_question('match'); $match->stems[5] = "Snake"; $match->stemsformat[5] = FORMAT_HTML; $match->choices[5] = "Reptile"; @@ -226,7 +226,7 @@ class qtype_match_question_test extends advanced_testcase { } public function test_prepare_simulated_post_data() { - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->start_attempt(new question_attempt_step(), 1); $postdata = $m->prepare_simulated_post_data(array('Dog' => 'Mammal', 'Frog' => 'Amphibian', 'Toad' => 'Amphibian', 'Cat' => 'Mammal')); diff --git a/question/type/match/tests/walkthrough_test.php b/question/type/match/tests/walkthrough_test.php index f5bd7af8b7f..b7930406041 100644 --- a/question/type/match/tests/walkthrough_test.php +++ b/question/type/match/tests/walkthrough_test.php @@ -41,7 +41,7 @@ class qtype_match_walkthrough_test extends qbehaviour_walkthrough_test_base { public function test_deferred_feedback_unanswered() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->shufflestems = false; $this->start_attempt_at_question($m, 'deferredfeedback', 4); @@ -98,7 +98,7 @@ class qtype_match_walkthrough_test extends qbehaviour_walkthrough_test_base { public function test_deferred_feedback_partial_answer() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->shufflestems = false; $this->start_attempt_at_question($m, 'deferredfeedback', 4); @@ -155,7 +155,7 @@ class qtype_match_walkthrough_test extends qbehaviour_walkthrough_test_base { public function test_interactive_correct_no_submit() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->hints = array( new question_hint_with_parts(11, 'This is the first hint.', FORMAT_HTML, false, false), new question_hint_with_parts(12, 'This is the second hint.', FORMAT_HTML, true, true), @@ -209,7 +209,7 @@ class qtype_match_walkthrough_test extends qbehaviour_walkthrough_test_base { public function test_interactive_partial_no_submit() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->hints = array( new question_hint_with_parts(11, 'This is the first hint.', FORMAT_HTML, false, false), new question_hint_with_parts(12, 'This is the second hint.', FORMAT_HTML, true, true), @@ -263,7 +263,7 @@ class qtype_match_walkthrough_test extends qbehaviour_walkthrough_test_base { public function test_interactive_with_invalid() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->hints = array( new question_hint_with_parts(11, 'This is the first hint.', FORMAT_HTML, false, false), new question_hint_with_parts(12, 'This is the second hint.', FORMAT_HTML, true, true), @@ -333,7 +333,7 @@ class qtype_match_walkthrough_test extends qbehaviour_walkthrough_test_base { public function test_match_with_tricky_html_choices() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->stems = array( 1 => '(1, 2]', 2 => '[1, 2]', @@ -387,7 +387,7 @@ class qtype_match_walkthrough_test extends qbehaviour_walkthrough_test_base { public function test_match_clear_wrong() { // Create a matching question. - $m = test_question_maker::make_a_matching_question(); + $m = test_question_maker::make_question('match'); $m->hints = array( new question_hint_with_parts(11, 'This is the first hint.', FORMAT_HTML, false, true), new question_hint_with_parts(12, 'This is the second hint.', FORMAT_HTML, true, true), From 9440b54f8a379dd8568b1b81b8a38b12d9eb167b Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 11 Mar 2020 12:28:39 +0000 Subject: [PATCH 2/2] MDL-68149 qtype_match: correctly distinguish 0 and 0.0 answers Also added a test for backup & restore (which was working). --- question/engine/tests/helpers.php | 2 +- question/type/match/questiontype.php | 3 +- question/type/match/tests/backup_test.php | 87 +++++++++++++ question/type/match/tests/helper.php | 115 +++++++++++++++++- .../type/match/tests/questiontype_test.php | 20 +++ 5 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 question/type/match/tests/backup_test.php diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index 32cd480597a..8443178a075 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -409,7 +409,7 @@ class test_question_maker { * Add some standard overall feedback to a question. You need to use these * specific feedback strings for the corresponding contains_..._feedback * methods in {@link qbehaviour_walkthrough_test_base} to works. - * @param question_definition $q the question to add the feedback to. + * @param question_definition|stdClass $q the question to add the feedback to. */ public static function set_standard_combined_feedback_fields($q) { $q->correctfeedback = self::STANDARD_OVERALL_CORRECT_FEEDBACK; diff --git a/question/type/match/questiontype.php b/question/type/match/questiontype.php index ffc9b4d455e..7e35d45ed92 100644 --- a/question/type/match/questiontype.php +++ b/question/type/match/questiontype.php @@ -124,8 +124,7 @@ class qtype_match extends question_type { $question->right = array(); foreach ($questiondata->options->subquestions as $matchsub) { - $ans = $matchsub->answertext; - $key = array_search($matchsub->answertext, $question->choices); + $key = array_search($matchsub->answertext, $question->choices, true); if ($key === false) { $key = $matchsub->id; $question->choices[$key] = $matchsub->answertext; diff --git a/question/type/match/tests/backup_test.php b/question/type/match/tests/backup_test.php new file mode 100644 index 00000000000..ec34d0b085a --- /dev/null +++ b/question/type/match/tests/backup_test.php @@ -0,0 +1,87 @@ +. + +/** + * Tests for the matching question type backup and restore logic. + * + * @package qtype_match + * @copyright 2020 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later. + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); +require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); +require_once($CFG->dirroot . '/course/externallib.php'); + + +/** + * Tests for the matching question type backup and restore logic. + */ +class qtype_match_backup_testcase extends advanced_testcase { + + /** + * Duplicate quiz with a matching question, and check it worked. + */ + public function test_duplicate_match_question() { + global $DB; + $this->resetAfterTest(); + $this->setAdminUser(); + + $coregenerator = $this->getDataGenerator(); + $questiongenerator = $coregenerator->get_plugin_generator('core_question'); + + // Create a course with a page that embeds a question. + $course = $coregenerator->create_course(); + $quiz = $coregenerator->create_module('quiz', ['course' => $course->id]); + $quizcontext = context_module::instance($quiz->cmid); + + $cat = $questiongenerator->create_question_category(['contextid' => $quizcontext->id]); + $question = $questiongenerator->create_question('match', 'trickynums', ['category' => $cat->id]); + + // Store some counts. + $numquizzes = count(get_fast_modinfo($course)->instances['quiz']); + $nummatchquestions = $DB->count_records('question', ['qtype' => 'match']); + + // Duplicate the page. + duplicate_module($course, get_fast_modinfo($course)->get_cm($quiz->cmid)); + + // Verify the copied quiz exists. + $this->assertCount($numquizzes + 1, get_fast_modinfo($course)->instances['quiz']); + + // Verify the copied question. + $this->assertEquals($nummatchquestions + 1, $DB->count_records('question', ['qtype' => 'match'])); + $newmatchid = $DB->get_field_sql(" + SELECT MAX(id) + FROM {question} + WHERE qtype = ? + ", ['match']); + $matchdata = question_bank::load_question_data($newmatchid); + + $subquestions = array_values($matchdata->options->subquestions); + + $this->assertSame('System.out.println(0);', $subquestions[0]->questiontext); + $this->assertSame('0', $subquestions[0]->answertext); + + $this->assertSame('System.out.println(0.0);', $subquestions[1]->questiontext); + $this->assertSame('0.0', $subquestions[1]->answertext); + + $this->assertSame('', $subquestions[2]->questiontext); + $this->assertSame('NULL', $subquestions[2]->answertext); + } +} diff --git a/question/type/match/tests/helper.php b/question/type/match/tests/helper.php index ee5ab4dd909..b93f3da29ff 100644 --- a/question/type/match/tests/helper.php +++ b/question/type/match/tests/helper.php @@ -37,7 +37,7 @@ require_once($CFG->dirroot . '/question/type/match/question.php'); */ class qtype_match_test_helper extends question_test_helper { public function get_test_questions() { - return array('foursubq'); + return array('foursubq', 'trickynums'); } /** @@ -159,4 +159,117 @@ class qtype_match_test_helper extends question_test_helper { return $match; } + /** + * Makes a matching question with choices including '0' and '0.0'. + * + * @return object the question definition data, as it might be returned from + * get_question_options. + */ + public function get_match_question_data_trickynums() { + global $USER; + + $q = new stdClass(); + test_question_maker::initialise_question_data($q); + $q->name = 'Java matching'; + $q->qtype = 'match'; + $q->parent = 0; + $q->questiontext = 'What is the output of each of these lines of code?'; + $q->questiontextformat = FORMAT_HTML; + $q->generalfeedback = 'Java has some advantages over PHP I guess!'; + $q->generalfeedbackformat = FORMAT_HTML; + $q->defaultmark = 1; + $q->penalty = 0.3333333; + $q->length = 1; + $q->hidden = 0; + $q->createdby = $USER->id; + $q->modifiedby = $USER->id; + + $q->options = new stdClass(); + $q->options->shuffleanswers = 1; + test_question_maker::set_standard_combined_feedback_fields($q->options); + + $q->options->subquestions = array( + 14 => (object) array( + 'id' => 14, + 'questiontext' => 'System.out.println(0);', + 'questiontextformat' => FORMAT_HTML, + 'answertext' => '0'), + 15 => (object) array( + 'id' => 15, + 'questiontext' => 'System.out.println(0.0);', + 'questiontextformat' => FORMAT_HTML, + 'answertext' => '0.0'), + 16 => (object) array( + 'id' => 16, + 'questiontext' => '', + 'questiontextformat' => FORMAT_HTML, + 'answertext' => 'NULL'), + ); + + return $q; + } + + /** + * Makes a match question about completing two blanks in some text. + * @return object the question definition data, as it might be returned from + * the question editing form. + */ + public function get_match_question_form_data_trickynums() { + $q = new stdClass(); + $q->name = 'Java matching'; + $q->questiontext = ['text' => 'What is the output of each of these lines of code?', 'format' => FORMAT_HTML]; + $q->generalfeedback = ['text' => 'Java has some advantages over PHP I guess!', 'format' => FORMAT_HTML]; + $q->defaultmark = 1; + $q->penalty = 0.3333333; + + $q->shuffleanswers = 1; + test_question_maker::set_standard_combined_feedback_form_data($q); + + $q->subquestions = array( + 0 => array('text' => 'System.out.println(0);', 'format' => FORMAT_HTML), + 1 => array('text' => 'System.out.println(0.0);', 'format' => FORMAT_HTML), + 2 => array('text' => '', 'format' => FORMAT_HTML), + ); + + $q->subanswers = array( + 0 => '0', + 1 => '0.0', + 2 => 'NULL', + ); + + $q->noanswers = 3; + + return $q; + } + + /** + * Makes a matching question with choices including '0' and '0.0'. + * + * @return qtype_match_question + */ + public static function make_match_question_trickynums() { + question_bank::load_question_definition_classes('match'); + $match = new qtype_match_question(); + test_question_maker::initialise_a_question($match); + $match->name = 'Java matching'; + $match->questiontext = 'What is the output of each of these lines of code?'; + $match->generalfeedback = 'Java has some advantages over PHP I guess!'; + $match->qtype = question_bank::get_qtype('match'); + + $match->shufflestems = 1; + + test_question_maker::set_standard_combined_feedback_fields($match); + + // Using unset to get 1-based arrays. + $match->stems = array('', 'System.out.println(0);', 'System.out.println(0.0);'); + $match->stemformat = array('', FORMAT_HTML, FORMAT_HTML); + $match->choices = array('', '0', '0.0', 'NULL'); + $match->right = array('', 1, 2); + unset($match->stems[0]); + unset($match->stemformat[0]); + unset($match->choices[0]); + unset($match->right[0]); + + return $match; + } } diff --git a/question/type/match/tests/questiontype_test.php b/question/type/match/tests/questiontype_test.php index d60a0b4fb8d..baaf44daa21 100644 --- a/question/type/match/tests/questiontype_test.php +++ b/question/type/match/tests/questiontype_test.php @@ -112,6 +112,26 @@ class qtype_match_test extends advanced_testcase { $this->assertTrue($this->qtype->can_analyse_responses()); } + public function test_make_question_instance() { + $questiondata = test_question_maker::get_question_data('match', 'trickynums'); + $question = question_bank::make_question($questiondata); + $this->assertEquals($questiondata->name, $question->name); + $this->assertEquals($questiondata->questiontext, $question->questiontext); + $this->assertEquals($questiondata->questiontextformat, $question->questiontextformat); + $this->assertEquals($questiondata->generalfeedback, $question->generalfeedback); + $this->assertEquals($questiondata->generalfeedbackformat, $question->generalfeedbackformat); + $this->assertInstanceOf('qtype_match', $question->qtype); + $this->assertEquals($questiondata->options->shuffleanswers, $question->shufflestems); + + $this->assertEquals( + [14 => 'System.out.println(0);', 15 => 'System.out.println(0.0);'], + $question->stems); + + $this->assertEquals([14 => '0', 15 => '0.0', 16 => 'NULL'], $question->choices); + + $this->assertEquals([14 => 14, 15 => 15], $question->right); + } + public function test_get_random_guess_score() { $q = $this->get_test_question_data(); $this->assertEquals(0.3333333, $this->qtype->get_random_guess_score($q), '', 0.0000001);