Moodle
  1. Moodle
  2. MDL-35388

Cannot run PHP unit tests for the new assignment module web services

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.4
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide

      To reproduce the problem:
      Run the php unit tests for the assignment web services created in MDL-31873 or MDL-31683. The tests will fail with a message saying that function assign_add_instance cannot accept a null value
      To test that the modifications in this issue have resolved the issue:
      Run the php unit tests again. The tests should run successfully

      Show
      To reproduce the problem: Run the php unit tests for the assignment web services created in MDL-31873 or MDL-31683 . The tests will fail with a message saying that function assign_add_instance cannot accept a null value To test that the modifications in this issue have resolved the issue: Run the php unit tests again. The tests should run successfully
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
    • Rank:
      44064

      Description

      Two problems are preventing PHP unit tests from working. These are:

      1) The tests generator cannot create an instance of the assign module because the assign_add_instance constructor in mod/assign/lib.php does not allow a null mod_assign_mod_form parameter. This can be resolved with the following modification

      function assign_add_instance(stdClass $data, mod_assign_mod_form $form = null)

      2) mod/assign/locallib.php needs to be modified to resolve the problem described in MDL-27968. The proposed solution is to lazily initialise the renderer which could be done by creating a new private function get_output which performs the initialisation and then replacing all calls to $this->output with a call to get_output

        Issue Links

          Activity

          Hide
          Paul Charsley added a comment -

          Hi everyone, I have made the changes described plus fixed up some of the pre existing minor coding standard errors picked up by the code checker. This is now ready for peer review and integration.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi everyone, I have made the changes described plus fixed up some of the pre existing minor coding standard errors picked up by the code checker. This is now ready for peer review and integration. Cheers, Paul
          Hide
          Damyon Wiese added a comment -

          Hi Paul,

          I have had a look and the changes to lazy load the renderer look good - the only issue I found there was the phpdocs for get_output have @param instead of @return

          Can you also please take out the changes to clean up the code in unrelated parts of the module?

          Leaving these in would mean:
          a) This code would conflict with outstanding patches in areas unrelated to this bug
          b) This code could introduce regressions in areas unrelated to this bug (Even though they are mostly whitespace changes)

          I would like to do a blanket code cleanup of the entire module - probably right after the 2.4 release when there is little in the queue and a long time for testing before the next release.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Paul, I have had a look and the changes to lazy load the renderer look good - the only issue I found there was the phpdocs for get_output have @param instead of @return Can you also please take out the changes to clean up the code in unrelated parts of the module? Leaving these in would mean: a) This code would conflict with outstanding patches in areas unrelated to this bug b) This code could introduce regressions in areas unrelated to this bug (Even though they are mostly whitespace changes) I would like to do a blanket code cleanup of the entire module - probably right after the 2.4 release when there is little in the queue and a long time for testing before the next release. Regards, Damyon
          Hide
          Paul Charsley added a comment -

          Hi Damyon,

          Oh dear, I had a nasty feeling you might say that! Yes I'll take out the code clean up and submit again shortly.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, Oh dear, I had a nasty feeling you might say that! Yes I'll take out the code clean up and submit again shortly. Cheers, Paul
          Hide
          Damyon Wiese added a comment -

          Sorry for the hassle - it will be nice when its all clean.

          Show
          Damyon Wiese added a comment - Sorry for the hassle - it will be nice when its all clean.
          Hide
          Paul Charsley added a comment -

          Hi Damyon, I've made the changes. Hopefully its now ready for integration!
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, I've made the changes. Hopefully its now ready for integration! Cheers, Paul
          Hide
          Damyon Wiese added a comment -

          Hi Paul,

          I just rebased to integration and added a minor cleanup patch on top as there were 2 functions to get the renderer.

          Can you provide testing instructions for this?

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Paul, I just rebased to integration and added a minor cleanup patch on top as there were 2 functions to get the renderer. Can you provide testing instructions for this? Regards, Damyon
          Hide
          Damyon Wiese added a comment -

          Thanks Paul

          Show
          Damyon Wiese added a comment - Thanks Paul
          Hide
          Paul Charsley added a comment -

          Hi Damyon,

          I've added testing instructions. Please could you submit MDL-31873 and MDL-31683 for integration since they will also be needed to test this.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, I've added testing instructions. Please could you submit MDL-31873 and MDL-31683 for integration since they will also be needed to test this. Cheers, Paul
          Hide
          Michael de Raadt added a comment -

          Thanks for your work on this, guys.

          Just a note that Unit tests are run automatically during integration, so the testing instructions could be simplified.

          If the integration of this issue is blocked by those other two issues (not clear to me), please create links of type "is blocked by" to indicate this to the integrators.

          Show
          Michael de Raadt added a comment - Thanks for your work on this, guys. Just a note that Unit tests are run automatically during integration, so the testing instructions could be simplified. If the integration of this issue is blocked by those other two issues (not clear to me), please create links of type "is blocked by" to indicate this to the integrators.
          Hide
          Damyon Wiese added a comment -

          Thanks Michael, it's the other way around - those issues have unit tests which fail because of this issue (so they depend on this).

          Show
          Damyon Wiese added a comment - Thanks Michael, it's the other way around - those issues have unit tests which fail because of this issue (so they depend on this).
          Hide
          Michael de Raadt added a comment -

          OK. Thanks, Damyon.

          We can create links the other way around, then. I will do that.

          Show
          Michael de Raadt added a comment - OK. Thanks, Damyon. We can create links the other way around, then. I will do that.
          Hide
          Eloy Lafuente (stronk7) 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.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) 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. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          I've integrated this now, thanks

          Show
          Dan Poltawski added a comment - I've integrated this now, thanks
          Hide
          Tim Barker added a comment -

          PHPUnit tests are run automatically and there are no failures so this can pass. Congrats

          Show
          Tim Barker added a comment - PHPUnit tests are run automatically and there are no failures so this can pass. Congrats
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: