moodleou / moodle-quiz_answersheets

A Moodle quiz report to allow quiz attempts to be exported
8 stars 15 forks source link

Core unit tests are failing if one of the supported qtypes is not in the code base #35

Open dmitriim opened 1 year ago

dmitriim commented 1 year ago

Running core unit tests in Moodle 4.1 while qtype_combined is not in a code base fails with following error.

PHP Warning:  Uncaught require_once(/var/www/moodle41/question/type/combined/renderer.php): failed to open stream: No such file or directory

/var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php:39
/var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php:39
/var/www/moodle41/lib/classes/component.php:137
/var/www/moodle41/lib/classes/component.php:909
/var/www/moodle41/lib/tests/component_test.php:533
/var/www/moodle41/lib/phpunit/classes/advanced_testcase.php:80
phpvfscomposer:///var/www/moodle41/vendor/phpunit/phpunit/phpunit:97

  thrown in /var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php on line 39
PHP Fatal error:  core_component::main(): Failed opening required '/var/www/moodle41/question/type/combined/renderer.php' (include_path='/var/www/moodle41/lib/pear:.:/usr/share/php') in /var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php on line 39

Warning: Uncaught require_once(/var/www/moodle41/question/type/combined/renderer.php): failed to open stream: No such file or directory

/var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php:39
/var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php:39
/var/www/moodle41/lib/classes/component.php:137
/var/www/moodle41/lib/classes/component.php:909
/var/www/moodle41/lib/tests/component_test.php:533
/var/www/moodle41/lib/phpunit/classes/advanced_testcase.php:80
phpvfscomposer:///var/www/moodle41/vendor/phpunit/phpunit/phpunit:97

  thrown in /var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php on line 39

Fatal error: core_component::main(): Failed opening required '/var/www/moodle41/question/type/combined/renderer.php' (include_path='/var/www/moodle41/lib/pear:.:/usr/share/php') in /var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php on line 39
dmitriim commented 1 year ago

looks like it fails on other not installed qtypes like oumultiresponse:

Fatal error: core_component::main(): Failed opening required '/var/www/moodle41/question/type/oumultiresponse/combinable/renderer.php' (include_path='/var/www/moodle41/lib/pear:.:/usr/share/php') in /var/www/moodle41/mod/quiz/report/answersheets/classes/output/combined/renderer.php on line 41

timhunt commented 1 year ago

This should work ... and, github actions runs the tests for this plugin without the otptional qtypes installed.

So, why is it failing for you but not github actions?

dmitriim commented 1 year ago

I guess this is because CI runs tests in isolation without running core tests. Try to install the plugin into vanilla moodle and run vendor/bin/phpunit lib/tests/component_test.php.

I think everything was explained in https://github.com/moodleou/moodle-quiz_answersheets/pull/36 that was closed.

If you don't want to reorganise the code, then the plugin needs to declare dependencies on all plugins it tires to include files from.

Any other ideas about fixing this issue?

timhunt commented 1 year ago

36 did not explain anything. At least not in a way I could understand. It asserted what it was doing, and what problem it was trying to solve, but it did not explain why that action achieves the goal. (I expect it is clear in your head, having just diagnosed the problem, but I am not telepathic. I am, however, busy and tired, so please make it easy for me.)

dmitriim commented 1 year ago

No worries, I'll provide more details. test_get_component_classes_in_namespace tries to load all classes for all plugins in output namespace here https://github.com/moodle/moodle/blob/master/lib/tests/component_test.php#L535 This at some point runs class_exists function with $autoload set to default true value. So it tries to load a class and as part of the code also tries to include not existing file which triggers PHP to bark with PHP Fatal error.

dmitriim commented 1 year ago

So as technically all qtypes renderers could be in any folder as in utils::get_question_renderer method we require a renderer file regardless. I think the easiest fix would be to move file around and make core tests happy.

Hope that explains everything now.

dmitriim commented 1 year ago

Hi @timhunt I know that you are busy with stuff related to 4.2 release, but is there any chance you can have a look at this one and revaluate https://github.com/moodleou/moodle-quiz_answersheets/pull/36 ? Thanks!

timhunt commented 1 year ago

This is still not very clear to me, so I can't jsut simply merge this.

The problem with moving stuff around is that we might (probalby do) have chagnes in our local codebase that we have not had time to push to github, and know, i don't have time to synch the two repos right now.

Are we sure this is not a bug in test_get_component_classes_in_namespace that should be fixed, rather than us working around it?

Or, if we need a work-around, would smoething like https://moodledev.io/docs/devupdate#developer-tip---handling-changes-to-base-class-names-while-supporting-multiple-moodle-versions be a less invasive (but more horrible) work-around?

dmitriim commented 1 year ago

I understand your concerns @timhunt

The main issue here is that auto-loaded files in output namespace are trying to include render files that potentially do not exist. So basically we auto-load a class that extends another class that potentially is not exist.

I don't think that it's a bug in test_get_component_classes_in_namespace as the test calls a core function to return all classes in a given namespace.

We can potentially do something like following so all files will remain where they are now. But I'm not sure if it's a good way also.

if (file_exists($CFG->dirroot . '/question/type/oumultiresponse/renderer.php')) {
    class qtype_oumultiresponse_override_renderer extends \qtype_oumultiresponse_renderer {   
       .... 
    }  
}
TomoTsuyuki commented 9 months ago

Hi @timhunt ,

I made PR https://github.com/moodleou/moodle-quiz_answersheets/pull/37

Could you review and merge if it's ok?