Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37835

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

    Details

      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.

        Gliffy Diagrams

          Activity

          Hide
          damyon Damyon Wiese added a comment -

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

          Show
          damyon Damyon Wiese added a comment - Thanks for the report and the patch. I'll peer review this asap.
          Hide
          eriklundberg 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
          eriklundberg 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 Damyon Wiese added a comment -

          URL Submission plugin (just an example).

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

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

          Show
          damyon Damyon Wiese added a comment - Does not apply to 23 because there was no generator class.
          Hide
          damyon 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 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 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 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

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

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

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

          Ciao

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

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

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

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

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

          Integrated (24 & master), thanks!

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

          This is working as expected.

          Tested for 2.4 and master.

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This is working as expected. Tested for 2.4 and master. Test passed.
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                11/Mar/13