Automated issues with MDL-83541 #167

[tlock]

Description of bug / unexpected behavior

When running all the Unit Test specifically quiz test related to MDL-83451 it has been failing with the new changes required in many third party plugins:

1. mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "formulas" ('formulas', 'testsinglenum')

Looking further, the test case in quiz is slightly different to the same stamp based tests in your plugin.

"Restore a course to another course having questions with the same stamp in a shared question bank context category."

Something is missing or different to other question types that aren't failing under this unit test.

Expected behavior

Quiz unit tests to pass testing the data providers for question Formulas.

How to reproduce the issue

vendor/bin/phpunit mod/quiz/tests/backup/repeated_restore_test.php --stop-on-failure

The unit tests for the question type all pass which leads to assume there is a case that hasn't been handled in the question type.

Plugin: 5.3.6 for Moodle 3.9 - 4.5 Moodle 4.5.4+ (Build: 20250424), 4713977b7d8241339d6e7c7067ab9139d1323aa8 Php: 8.3.16, pgsql: 15.8 (Postgres.app), OS: Darwin 24.4.0 arm64 PHPUnit 9.6.18 by Sebastian Bergmann and contributors.

.....SSSS.SSS...SS....S..S.....................S............... 63 / 135 ( 46%) .........S.......SSSS.SSSF..SS....S..S....S..S.SSS.........SS.. 126 / 135 ( 93%) S........ 135 / 135 (100%)

Time: 03:48.906, Memory: 159.00 MB

There was 1 failure:

1. mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "formulas" ('formulas', 'testsinglenum') Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'3' +'4'

mod/quiz/tests/backup/repeated_restore_test.php:572 lib/phpunit/classes/advanced_testcase.php:83

FAILURES! Tests: 135, Assertions: 275, Failures: 1, Skipped: 32.

Environment

System Details

• Moodle version: Moodle 4.5.4+ (Build: 20250424),
• Plugin version: 2025042900 (5.3.6)
• PHP version: Php: 8.3.16, pgsql: 15.8 (Postgres.app), OS: Darwin 24.4.0 arm64
• Browser: N/A

Additional comments

I have been able to solve other testing issues with other question types, but unfortunately not in this instance.

Let me know if you need anything further.

[PhilippImhof]

Thanks @tlock for your very detailed report. I am a bit surprised, because I used the "official" unit tests to test my plugin when I implemented the changes that MDL-83541 made necessary. But I will, of course, look into it very carefully.

[PhilippImhof]

I have looked into it again and found the source of the problem. As you correctly pointed out, our own version of the test function differs from the one in Moodle core. The core test function does this:

https://github.com/moodle/moodle/blob/4d4369e073980298991a4c9478dd8e998e5518ec/mod/quiz/tests/backup/repeated_restore_test.php#L548-L558

        // Change the answers of the question2 to be different to question1.
        $question2data = \question_bank::load_question_data($question2->id);
        if (!isset($question2data->options->answers) || empty($question2data->options->answers)) {
            $this->markTestSkipped(
                "Cannot test edited answers for qtype_{$qtype} as it does not use answers.",
            );
        }
        foreach ($question2data->options->answers as $answer) {
            $answer->answer = 'New answer ' . $answer->id;
            $DB->update_record('question_answers', $answer);
        }

Now here's the thing: The Formulas question type does not use the question_answers table. Instead, it has a specific table called qtype_formulas_answers. The core unit test wrongfully assumes that, as there is something in $question2data->options->answers, it can modify the data by changing the entry in the question_answers table in the database. (I do not blame them, because that test is mainly done for the core question types and it serves as a very good base for plugin authors.)

In our own unit test, we apply the change to the appropriate table instead. That's why the test passes. Now to confirm that this line is indeed responsible for the failing test, I commented it out in the core test and ran it with core question types:

$ vendor/bin/phpunit mod/quiz/tests/backup/repeated_restore_test.php --filter '.*test_restore_course_with_same_stamp.*'                  

Moodle 4.5.4 (Build: 20250414), 8f0c7bb53cd3ead755e941183e6d3bd0ac717a19
Php: 8.2.25, mysqli: 8.4.3, OS: Darwin 23.6.0 arm64
PHPUnit 9.6.18 by Sebastian Bergmann and contributors.

FFFSSFSSFFSSFFFSFF                                                18 / 18 (100%)

Time: 00:12.793, Memory: 92.50 MB

There were 11 failures:

1) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "calculated" ('calculated', 'sum')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469000'
+'469001'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

2) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "calculatedmulti" ('calculatedmulti', 'singleresponse')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469002'
+'469003'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

3) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "calculatedsimple" ('calculatedsimple', 'sumwithvariants')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469004'
+'469005'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

4) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "ddwtos" ('ddwtos', 'fox')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469010'
+'469011'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

5) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "formulas" ('formulas', 'testsinglenum')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469016'
+'469017'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

6) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "gapselect" ('gapselect', 'missingchoiceno')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469018'
+'469019'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

7) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "multichoice" ('multichoice', 'two_of_four')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469028'
+'469029'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

8) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "numerical" ('numerical', 'pi')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469030'
+'469031'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

9) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "ordering" ('ordering', 'moodle')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469032'
+'469033'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

10) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "shortanswer" ('shortanswer', 'frogtoad')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469034'
+'469035'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

11) mod_quiz\backup\repeated_restore_test::test_restore_course_with_same_stamp_questions with data set "truefalse" ('truefalse', 'true')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'469036'
+'469037'

/moodle/mod/quiz/tests/backup/repeated_restore_test.php:574
/moodle/lib/phpunit/classes/advanced_testcase.php:76

FAILURES!
Tests: 18, Assertions: 11, Failures: 11, Skipped: 7.

As you can see, all core question types fail this unit test, if the answer is not changed in the database. Therefore, as this test is not able to change the Formulas question's answer, it will always fail for this plugin.

[tlock]

Hi @PhilippImhof ,

Thanks for providing the background I was lacking. Are you able to publish your finding as regression of MDL-83541? I'm sure your plugin isn't the only one using custom answers table that would be affected by this change.

Core Test should implement if use custom answers table, get table name, else use the default and problem is resolved as you noted.

Regards, Tim

[PhilippImhof]

I have briefly discussed this in the Moodle developer chat. I will open an issue in the tracker and submit a patch to change the core unit test. Therefore, I have reopened the issue, because that seems to be an easy way to keep you posted on the progress.

[PhilippImhof]

@tlock I have opened MDL-85350. While preparing the change, I discovered that there is a fundamental difference between 4.5 and 5.0 or main. The tests do not fail on 5.0 or main; that's probably also why I never had them failing before. I contacted the Moodle developer who wrote the test in order to investigate that. (It might mean that the test does not actually work correctly on 5.0 and main.)

[tlock]

Thanks for the followup.

I'm going to fix the core unit test, if formulas, use your table else use the core table.

[PhilippImhof]

You mean, you do that locally?

I have a patch ready for core, but I am waiting for feedback from the Moodle developers.

[tlock]

Yes because if a build fails any unit test we don't apply the change to our clients which we have been in the holding pattern since the MDL item dropped.

Great news, we'll get the improvement then undo our workaround.

[PhilippImhof]

@tlock A quick update: Due to some changes to the question bank from Moodle 5.0 onwards, the test behaves differently for 4.5 and 5.0. This problem is addressed in MDL-85556 which will probably be merged very soon.

Once this is done, I will add my fix for the test in order for it to skip Formulas questions (and others that do not use the question_answers table).

[PhilippImhof]

@tlock Just to let you know that the fix for MDL-85556 is currently in integration review. I will keep you posted.

[tlock]

Hi Phil,

Thanks for the followup.

[PhilippImhof]

MDL-85556 has been integrated and closed. I have updated MDL-85350 with a patch that would fix the issue by skipping over 3rd party plugins that use answers, but do not store them in the question_answers table. Once the automated checks are done, I will submit it for peer-review.

[PhilippImhof]

Hello @tlock

My patch for MDL-85350 has been integrated and tested, it will be available in Moodle's public repositories very soon, probably by Monday.

Best regards

[tlock]

Hi @PhilippImhof ,

Thanks for the followup.

[PhilippImhof]

@tlock The fix is now included in the official Moodle repositories and branches. I am therefore closing this; if anything goes wrong, please do not hesitate to open it again.