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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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:

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            pcharsle 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
            pcharsle 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 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 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
            pcharsle 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
            pcharsle 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 Damyon Wiese added a comment -

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

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

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

            Show
            pcharsle Paul Charsley added a comment - Hi Damyon, I've made the changes. Hopefully its now ready for integration! Cheers, Paul
            Hide
            damyon 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 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 Damyon Wiese added a comment -

            Thanks Paul

            Show
            damyon Damyon Wiese added a comment - Thanks Paul
            Hide
            pcharsle 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
            pcharsle 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
            salvetore 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
            salvetore 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 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 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
            salvetore Michael de Raadt added a comment -

            OK. Thanks, Damyon.

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

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

            I've integrated this now, thanks

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

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

            Show
            timb Tim Barker added a comment - PHPUnit tests are run automatically and there are no failures so this can pass. Congrats
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  3/Dec/12