From e3bfcb1cde1335cce2cc6632587cb93a62bd876e Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 9 Jul 2020 22:30:18 +0100 Subject: [PATCH] MDL-69246 questions: allow for float issues when validating manual marks As well as fixing the bug, I also rewrote the test to use data providers, which should lead to more useful failure messages. And, I moved the magic number we used as the float tolerence to be a named constant. --- question/behaviour/behaviourbase.php | 2 +- question/engine/lib.php | 10 ++++ question/engine/questionattempt.php | 5 +- .../tests/questionattempt_with_steps_test.php | 54 ++++++++++++++----- 4 files changed, 54 insertions(+), 17 deletions(-) diff --git a/question/behaviour/behaviourbase.php b/question/behaviour/behaviourbase.php index 1b3ac4b6dd0..090d11433ff 100644 --- a/question/behaviour/behaviourbase.php +++ b/question/behaviour/behaviourbase.php @@ -784,7 +784,7 @@ abstract class question_cbm { // errors for people who have used TGM's patches. return 0; } - if ($fraction <= 0.00000005) { + if ($fraction <= question_utils::MARK_TOLERANCE) { return self::$wrongscore[$certainty]; } else { return self::$rightscore[$certainty] * $fraction; diff --git a/question/engine/lib.php b/question/engine/lib.php index edd8c6db11f..cc8ebc2b473 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -790,6 +790,16 @@ class question_out_of_sequence_exception extends moodle_exception { * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ abstract class question_utils { + /** + * @var float tolerance to use when comparing question mark/fraction values. + * + * When comparing floating point numbers in a computer, the representation is not + * necessarily exact. Therefore, we need to allow a tolerance. + * Question marks are stored in the database as decimal numbers with 7 decimal places. + * Therefore, this is the appropriate tolerance to use. + */ + const MARK_TOLERANCE = 0.00000005; + /** * Tests to see whether two arrays have the same keys, with the same values * (as compared by ===) for each key. However, the order of the arrays does diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index 0ab9995e6de..ba41f67118a 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -695,7 +695,7 @@ class question_attempt { /** @return bool whether this question attempt has a non-zero maximum mark. */ public function has_marks() { // Since grades are stored in the database as NUMBER(12,7). - return $this->maxmark >= 0.00000005; + return $this->maxmark >= question_utils::MARK_TOLERANCE; } /** @@ -1158,7 +1158,8 @@ class question_attempt { } $maxmark = $this->get_max_mark(); - if ($mark > $maxmark * $this->get_max_fraction() || $mark < $maxmark * $this->get_min_fraction()) { + if ($mark > $maxmark * $this->get_max_fraction() + question_utils::MARK_TOLERANCE || + $mark < $maxmark * $this->get_min_fraction() - question_utils::MARK_TOLERANCE) { return get_string('manualgradeoutofrange', 'question'); } diff --git a/question/engine/tests/questionattempt_with_steps_test.php b/question/engine/tests/questionattempt_with_steps_test.php index e8febb6dfeb..b5194c008ac 100644 --- a/question/engine/tests/questionattempt_with_steps_test.php +++ b/question/engine/tests/questionattempt_with_steps_test.php @@ -170,19 +170,45 @@ class question_attempt_with_steps_test extends advanced_testcase { $qa->get_max_fraction(); } - public function test_validate_manual_mark() { - $this->qa->set_min_fraction(0); - $this->qa->set_max_fraction(1); - $this->assertSame('', $this->qa->validate_manual_mark(null)); - $this->assertSame('', $this->qa->validate_manual_mark('')); - $this->assertSame('', $this->qa->validate_manual_mark('0')); - $this->assertSame('', $this->qa->validate_manual_mark('0.0')); - $this->assertSame('', $this->qa->validate_manual_mark('2,0')); - $this->assertSame(get_string('manualgradeinvalidformat', 'question'), - $this->qa->validate_manual_mark('frog')); - $this->assertSame(get_string('manualgradeoutofrange', 'question'), - $this->qa->validate_manual_mark('2.1')); - $this->assertSame(get_string('manualgradeoutofrange', 'question'), - $this->qa->validate_manual_mark('-0,01')); + /** + * Test cases for {@see test_validate_manual_mark()}. + * + * @return array test cases + */ + public function validate_manual_mark_cases(): array { + // Recall, the DB schema stores question grade information to 7 decimal places. + return [ + [0, 1, 2, null, ''], + [0, 1, 2, '', ''], + [0, 1, 2, '0', ''], + [0, 1, 2, '0.0', ''], + [0, 1, 2, '2,0', ''], + [0, 1, 2, 'frog', get_string('manualgradeinvalidformat', 'question')], + [0, 1, 2, '2.1', get_string('manualgradeoutofrange', 'question')], + [0, 1, 2, '-0,01', get_string('manualgradeoutofrange', 'question')], + [-0.3333333, 1, 0.75, '0.75', ''], + [-0.3333333, 1, 0.75, '0.7500001', get_string('manualgradeoutofrange', 'question')], + [-0.3333333, 1, 0.75, '-0.25', ''], + [-0.3333333, 1, 0.75, '-0.2500001', get_string('manualgradeoutofrange', 'question')], + ]; + } + + /** + * Test validate_manual_mark. + * + * @dataProvider validate_manual_mark_cases + * + * @param float $minfraction minimum fraction for the question being attempted. + * @param float $maxfraction maximum fraction for the question being attempted. + * @param float $maxmark marks for the question attempt. + * @param string|null $currentmark submitted mark. + * @param string $expectederror expected error, if any. + */ + public function test_validate_manual_mark(float $minfraction, float $maxfraction, + float $maxmark, ?string $currentmark, string $expectederror) { + $this->qa->set_min_fraction($minfraction); + $this->qa->set_max_fraction($maxfraction); + $this->qa->set_max_mark($maxmark); + $this->assertSame($expectederror, $this->qa->validate_manual_mark($currentmark)); } }