fatal error test failure in 3.11 after core change

aspark21 commented 2 years ago

Started having the following errors in MOODLE_311_STABLE against main branch of this plugin in the last week.

 PHP Fatal error:  Uncaught Error: Class 'mod_quiz_attempt_walkthrough_from_csv_testcase' not found in /var/www/html/question/type/varnumeric/tests/stats_from_steps_walkthrough_test.php:44
    Stack trace:
    #0 /var/www/html/vendor/phpunit/phpunit/src/Util/FileLoader.php(65): include_once()
    #1 /var/www/html/vendor/phpunit/phpunit/src/Util/FileLoader.php(49): PHPUnit\Util\FileLoader::load('/var/www/html/q...')
    #2 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(401): PHPUnit\Util\FileLoader::checkAndLoad('/var/www/html/q...')
    #3 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(529): PHPUnit\Framework\TestSuite->addTestFile('/var/www/html/q...')
    #4 /var/www/html/vendor/phpunit/phpunit/src/TextUI/TestSuiteMapper.php(67): PHPUnit\Framework\TestSuite->addTestFiles(Array)
    #5 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(390): PHPUnit\TextUI\TestSuiteMapper->map(Object(PHPUnit\TextUI\XmlConfiguration\TestSuiteCollection), '')
    #6 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(111): P in /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php on line 98

Timing didn't seem to relate to changes within this plugin but found this change in core Moodle that will be the cause:

https://github.com/moodle/moodle/commit/65c883915788a8c187aa398df31d1bed33f37a96#diff-c47499725b252b6e8bc56f93f82205e49df4014e2c06c508accf2937e2ab0e5cR39

Seems there's a collection of them: MDL-75716 MDL-75777 MDL-66902

aspark21 commented 2 years ago

ties back to https://tracker.moodle.org/browse/MDL-71049

timhunt commented 2 years ago

Yes, that will have broken the unit tests in this plugin. Please complain to Eloy in the tracker. If they moved base classes, they should have listed them as renamed classes.

stronk7 commented 2 years ago

Hi,

I tried not to rename base classes in stables but, obviously, it seems that some of them escaped to me. Surely when backporting or over the multiple (conflicting) weekly rebases I had to do with some of the branches, I'm afraid.

Problem with renamed classes (I remember I tried that with some other base class) is that they will emit debugging and surely will keep failing the unit tests.

And the alternative solution is about to re-introduce the original base classes, just extending the new ones. But this solution is not perfect either, because 4.1dev (master) won't get them re-introduced. So, if you are testing the same plugin codebase against multiple branches (like it seems that you are doing here, I can see 311 and master in the GHA workflow)... it will continue failing against master.

So I'm not sure how to proceed with this (core-wise) because no solution will work for plugins being tested against multiple core branches, unless the plugin, effectively, moves to the new base classes.

Uhm... I continue thinking about this... just sharing my first thoughts.

Ciao :-)

stronk7 commented 2 years ago

I annotate this here till there is an issue:

Ok, good news is that there are only 2 renamed/moved classes in 311_STABLE and only 1 in 400_STABLE potentially causing problems.

I've examined all test files changed since .0 release on every branch, removing those that haven't changed or that are core-core (not related with plugins or subplugins) and have got this:

311_STABLE:

git diff --name-only v3.11.0 MOODLE_311_STABLE | \
grep -P '_test.php|_testcase.php' | xargs grep -P '^class .*(_test|_testcase) extends .*' | \
grep -vP 'base_testcase|basic_testcase|advanced_testcase' | \
grep -vP 'restore_date_testcase|provider_testcase|data_privacy_testcase|cachestore_tests|messagelib_test' | \
grep -vP 'database_driver_testcase|core_reportbuilder_testcase|lti_advantage_testcase' | \
grep -vP 'mod_lti_testcase|repository_googledocs_testcase|googledocs_content_testcase' | \
grep -vP 'question_testcase|qbehaviour_walkthrough_test_base|question_attempt_upgrader_test_base' | \
grep -vP 'data_loading_method_test_base|all_checks|area_test_base|brickfield_accessibility_test'

\mod_assign\privacy\provider_test
\mod_quiz\attempt_walkthrough_from_csv_test

400_STABLE:

git diff --name-only v4.0.0 MOODLE_400_STABLE | \
grep -P '_test.php|_testcase.php' | xargs grep -P '^class .*(_test|_testcase) extends .*' | \
grep -vP 'base_testcase|basic_testcase|advanced_testcase' | \
grep -vP 'restore_date_testcase|provider_testcase|data_privacy_testcase|cachestore_tests|messagelib_test' | \
grep -vP 'database_driver_testcase|core_reportbuilder_testcase|lti_advantage_testcase' | \
grep -vP 'mod_lti_testcase|repository_googledocs_testcase|googledocs_content_testcase' | \
grep -vP 'question_testcase|qbehaviour_walkthrough_test_base|question_attempt_upgrader_test_base' | \
grep -vP 'data_loading_method_test_base|all_checks|area_test_base|brickfield_accessibility_test'

\mod_quiz\attempt_walkthrough_from_csv_test

Still thinking which is the best way to fix them... but at very least... they aren't legion.

Ciao :-)

stronk7 commented 2 years ago

Ok, some more background about the 2 cases found above:

\mod_assign\privacy\provider_test

Its original name was \mod_assign\tests\mod_assign_privacy_testcase
Was introduced by MDL-73348 and merged upstream in December 21.
For Moodle 3.11.5 and up.

\mod_quiz\attempt_walkthrough_from_csv_test

Its original name was \mod_quiz_attempt_walkthrough_from_csv_testcase
Was introduced by MDL-75716 and merged upstream in September 22.
For Moodle 3.11.11, 4.0.5 and up.

Now looking for known uses in pluginland... coming soon...

Edited, to fix links...

stronk7 commented 2 years ago

The following uses have been found in pluginland (examining all the checks that CiBoT runs against plugins):

\mod_assign\tests\mod_assign_privacy_testcase

assignfeedback_onenotelink: Already fixed (May 2022) - see commit.
assignsubmission_onenote: Already fixed (May 2022) - see commit.
assignsubmission_collabora: Already fixed (March 2022) - see commit.
assignsubmission_comparativejudgement: Not fixed - see commit. Isn't published in the plugins database. And was tested against 39_STABLE only.

\mod_quiz_attempt_walkthrough_from_csv_testcase

qtype_varnumeric: Not fixed - see commit. main branch only, it seems.

So, I'm certainly becoming biased about just fix it here and forget this unfortunate incident...

Alternatively, 100% up to you, feel free to create MDL issue and we'll see there how to make the old 2 classes available there (surely extending in stables and maybe adding to renamedclasses in master, so it shows debugging info). Although, as said... there aren't more uses (publicly available and published) that the ones above.

Ciao :-)

timhunt commented 2 years ago

Thanks for that research Eloy, in that case we will just fix it here.

nadavkav commented 1 year ago

Does it make sense to change: class qtype_varnumeric_statistics_from_steps_testcase extends mod_quiz_attempt_walkthrough_from_csv_testcase To: class qtype_varnumeric_statistics_from_steps_testcase extends \mod_quiz\attempt_walkthrough_from_csv_test

Which seems to fix this issue for me

timhunt commented 1 year ago

nadav, that only works in 4.2, and this plugin supports mulitple Moodle versions from its master branch.

nadavkav commented 1 year ago

@timhunt , it works for me in 4.1, but I get your point.

timhunt commented 8 months ago

I just pushed the latest OU code changes, which should have fixed this. Closing.

moodleou / moodle-qtype_varnumeric