Moodle
  1. Moodle
  2. MDL-37835

Error in unit tests when custom submission/feedback plugins are installed

    Details

    • Rank:
      47579

      Description

      If one or more custom (ie. not bundled with the assignment module) feedback or submission plugins are activated while running through the unit tests multiple errors like the one below will emerge.

      5) courselib_testcase::test_module_visibility
      Undefined property: stdClass::$assignsubmission_pdf_enabled

      /var/www/moodle24/mod/assign/locallib.php:645
      /var/www/moodle24/mod/assign/locallib.php:475
      /var/www/moodle24/mod/assign/lib.php:39
      /var/www/moodle24/mod/assign/tests/generator/lib.php:124
      /var/www/moodle24/lib/phpunit/classes/data_generator.php:430
      /var/www/moodle24/course/tests/courselib_test.php:325
      /var/www/moodle24/lib/phpunit/classes/advanced_testcase.php:76

      This is caused by the assignment generator which is responsible for creating assignments for testing purposes. The assignment generator does only set the $assignsubmission_PLUGINNAME_enabled/$assignfeedback_PLUGINNAME_enabled for the standard set of submission/feedback-plugins which causes undefined properaty-errors for other plugins.

      Replication steps:
      1. Install a custom submission or feedback plugin for the assign module.
      2. Run throgh the unit tests.

        Activity

        Hide
        Damyon Wiese added a comment -

        Thanks for the report and the patch. I'll peer review this asap.

        Show
        Damyon Wiese added a comment - Thanks for the report and the patch. I'll peer review this asap.
        Hide
        Erik Lundberg added a comment -

        Hi Damyon, thanks for your feedback.

        I investigated this issue in the master branch and found these assertions in the assignment test suite (/mod/assign/tests/locallib_test.php):

        public function test_get_feedback_plugins()

        { $this->setUser($this->editingteachers[0]); $assign = $this->create_instance(); $this->assertEquals(3, count($assign->get_feedback_plugins())); }

        public function test_get_submission_plugins()

        { $this->setUser($this->editingteachers[0]); $assign = $this->create_instance(); $this->assertEquals(3, count($assign->get_submission_plugins())); }

        From what I can see, these assertions can only work as long as the Moodle installation doesn't have any installed feedback/submission plugins. Would it be a better test if the assertions fetched the installed plugins in the database and compared against the result of the get_X_plugins()-method?

        Show
        Erik Lundberg added a comment - Hi Damyon, thanks for your feedback. I investigated this issue in the master branch and found these assertions in the assignment test suite (/mod/assign/tests/locallib_test.php): public function test_get_feedback_plugins() { $this->setUser($this->editingteachers[0]); $assign = $this->create_instance(); $this->assertEquals(3, count($assign->get_feedback_plugins())); } public function test_get_submission_plugins() { $this->setUser($this->editingteachers[0]); $assign = $this->create_instance(); $this->assertEquals(3, count($assign->get_submission_plugins())); } From what I can see, these assertions can only work as long as the Moodle installation doesn't have any installed feedback/submission plugins. Would it be a better test if the assertions fetched the installed plugins in the database and compared against the result of the get_X_plugins()-method?
        Hide
        Damyon Wiese added a comment -

        URL Submission plugin (just an example).

        Show
        Damyon Wiese added a comment - URL Submission plugin (just an example).
        Hide
        Damyon Wiese added a comment -

        Does not apply to 23 because there was no generator class.

        Show
        Damyon Wiese added a comment - Does not apply to 23 because there was no generator class.
        Hide
        Damyon Wiese added a comment -

        Thanks for the report and the patch Erik,

        I had to redo the patch for master because the code has changed since 24. I used your patch for 24 - but I amended the commit message to meet our coding guidelines.

        I also added a sample plugin to test this change (url submission plugin - very basic).

        Cheers, Damyon

        Show
        Damyon Wiese added a comment - Thanks for the report and the patch Erik, I had to redo the patch for master because the code has changed since 24. I used your patch for 24 - but I amended the commit message to meet our coding guidelines. I also added a sample plugin to test this change (url submission plugin - very basic). Cheers, Damyon
        Hide
        Damyon Wiese added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        Thanks!

        Show
        Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Please, amend the 2 "foreach(" in 24_STABLE ant this will happily land, TIA!

        Show
        Eloy Lafuente (stronk7) added a comment - Please, amend the 2 "foreach(" in 24_STABLE ant this will happily land, TIA!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And, uhm, the master branch contains 2 unrelated commits integrated last week (MDL-37329).

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And, uhm, the master branch contains 2 unrelated commits integrated last week ( MDL-37329 ). Ciao
        Hide
        Damyon Wiese added a comment -

        Both branches rebased and removed the nested foreach loops for 2.4.

        Show
        Damyon Wiese added a comment - Both branches rebased and removed the nested foreach loops for 2.4.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        lol, my comment for 24_STABLE was about the missing space in "foreach(". Amending it here...

        Show
        Eloy Lafuente (stronk7) added a comment - lol, my comment for 24_STABLE was about the missing space in "foreach(". Amending it here...
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (24 & master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks!
        Hide
        Rossiani Wijaya added a comment -

        This is working as expected.

        Tested for 2.4 and master.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is working as expected. Tested for 2.4 and master. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Because

        A
        MARVELOUS
        A       U
        Z  YOU  P
        I  ARE  E
        N  PPL  R
        G       B
          TNKS! 
        

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: