Moodle
  1. Moodle
  2. MDL-30210

Fix the broken web service unit tests

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.1.4, 2.2
    • Fix Version/s: STABLE backlog
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      Edit the /webservice/simpletest/testwebservice.php file:
      1- set to true all protocols.
      2- set to true all functions.
      3- Run the unit tests twice in the Moodle admin.

      Show
      Edit the /webservice/simpletest/testwebservice.php file: 1- set to true all protocols. 2- set to true all functions. 3- Run the unit tests twice in the Moodle admin.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      26464

      Description

      There are many issues with the web service unit tests that I fixed in https://github.com/mouneyrac/local_wstesting/blob/master/simpletest/testwebservice.php

      From memory the fixes are:

      • REST server does not accept object parameters. I casted all object into array.
      • some parameters name were incorrect. XML-RPC and SOAP were bypassing wrong name, only looking at the order. By REST was failing.
      • Some functions were creating trash data and failed on second run.
      • one of the new Moodle 2.2 web service function test is new: core_enrol_get_enrolled_users

      Fixing in for 2.2 will be easy (it's about copying the file). Backporting in 2.1/2.0 is just a bit more work. After copying the file, take care about putting back the old moodle_enrol_get_enrolled_users instead core_enrol_get_enrolled_users, and also remove the possibility to test REST server (REST-JSON server only available in 2.2).

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Hi Rosie, I assign it to you for peer-review. To run the REST tests, you need to cherry-pick http://tracker.moodle.org/browse/MDL-30126

          Show
          Jérôme Mouneyrac added a comment - Hi Rosie, I assign it to you for peer-review. To run the REST tests, you need to cherry-pick http://tracker.moodle.org/browse/MDL-30126
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Note I tested with 2.1 and HEAD (2.2 == HEAD). To test REST in 2.1 you have to cherry pick first the REST JSON server: http://tracker.moodle.org/browse/MDL-29279. There is a little conflict to fix but it's easy, it's just deleting the "HEAD" conflict code.

          Show
          Jérôme Mouneyrac added a comment - - edited Note I tested with 2.1 and HEAD (2.2 == HEAD). To test REST in 2.1 you have to cherry pick first the REST JSON server: http://tracker.moodle.org/browse/MDL-29279 . There is a little conflict to fix but it's easy, it's just deleting the "HEAD" conflict code.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          For the peer-review you don't need to list all the unit test code issues. I wanted to get rid of this 'dev testing' file (move it in my personal github rep), but it was refused during integration because it is good to have something to test in core. Moreover, I don't know yet what the situation is with unit test. Do we go with PHPUnit soon? I don't feel it right to spend much time with this file specially about its coding performance.

          However the interest of these unit tests are to prove that the different protocol are not broken. If all tests go green then valid the peer-review/integration.

          Thank you.

          Show
          Jérôme Mouneyrac added a comment - - edited For the peer-review you don't need to list all the unit test code issues. I wanted to get rid of this 'dev testing' file (move it in my personal github rep), but it was refused during integration because it is good to have something to test in core. Moreover, I don't know yet what the situation is with unit test. Do we go with PHPUnit soon? I don't feel it right to spend much time with this file specially about its coding performance. However the interest of these unit tests are to prove that the different protocol are not broken. If all tests go green then valid the peer-review/integration. Thank you.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          The unit test is pass in all 3 branches.

          However, here are some feedback (master branch) for this issue:

          1. line 214, 1114: will $groupids ever not empty?
          2. line 614 - exception with missing language string {testdatausersalreadyexist)
            #line 831 - $searchusers = $DB->get_records_list('user', 'username', array($user1->username, $user1->username)). Should the second array value come from $user2->username?)
            #line 872 - can't you just reuse $searchusers from line 831?
          3. line 1037 - $searchusers should only returning 1 user. $user = $searchusers

          I didn't look deeply for 2.1 version, as you suggested that it is unnecessary to do.

          Show
          Rossiani Wijaya added a comment - Hi Jerome, The unit test is pass in all 3 branches. However, here are some feedback (master branch) for this issue: line 214, 1114: will $groupids ever not empty? line 614 - exception with missing language string {testdatausersalreadyexist) #line 831 - $searchusers = $DB->get_records_list('user', 'username', array($user1->username, $user1->username)). Should the second array value come from $user2->username?) #line 872 - can't you just reuse $searchusers from line 831? line 1037 - $searchusers should only returning 1 user. $user = $searchusers I didn't look deeply for 2.1 version, as you suggested that it is unnecessary to do.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Rosie, 1) was a mistake that appeared during a code merging, I moved it few lines below as I wanted to do Submitting for integration.

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Rosie, 1) was a mistake that appeared during a code merging, I moved it few lines below as I wanted to do Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note this is halted till MDL-30126 is done...

          Show
          Eloy Lafuente (stronk7) added a comment - Note this is halted till MDL-30126 is done...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (sending back to integration, as agreed, thanks!)

          Show
          Eloy Lafuente (stronk7) added a comment - (sending back to integration, as agreed, thanks!)
          Hide
          Luis de Vasconcelos added a comment -

          Is MDL-31609 (Web Service Test Client only contains the deprecated moodle_* functions.) related to this in any way?

          Show
          Luis de Vasconcelos added a comment - Is MDL-31609 (Web Service Test Client only contains the deprecated moodle_* functions.) related to this in any way?
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Luis, the web service test client is another issue. When the web service test client will be fixed (if it is not removed) I suppose it will be an automatic one.

          Show
          Jérôme Mouneyrac added a comment - Hi Luis, the web service test client is another issue. When the web service test client will be fixed (if it is not removed) I suppose it will be an automatic one.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oh,

          I've applied this locally to 21, 22 and master. Then I've enabled all protocols and functions in testwebservice.php and also have activated everything within Moodle (protocols, services, user token, functions...).

          Then I've executed the tests for /webservice and got:

          • 21_STABLE: 1/1 test cases complete: 492 passes, 2 fails and 0 exceptions.
          • 22_STABLE: 1/1 test cases complete: 218 passes, 2 fails and 3 exceptions.
          • master: 1/1 test cases complete: 218 passes, 2 fails and 3 exceptions.

          Are those the expected results? It surprised me a lot to see that 21_STABLE had far more tests than 22_STABLE/master.

          Also, note that I had to reinstall one time my 22_STABLE dev server because the first execution ended deleting my admin user in the site, not sure why (perhaps I enabled AMF, not sure).

          Please, confirm if that's the expected behavior with the patch applied. And I will integrate / reject accordingly to that.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, I've applied this locally to 21, 22 and master. Then I've enabled all protocols and functions in testwebservice.php and also have activated everything within Moodle (protocols, services, user token, functions...). Then I've executed the tests for /webservice and got: 21_STABLE: 1/1 test cases complete: 492 passes, 2 fails and 0 exceptions. 22_STABLE: 1/1 test cases complete: 218 passes, 2 fails and 3 exceptions. master: 1/1 test cases complete: 218 passes, 2 fails and 3 exceptions. Are those the expected results? It surprised me a lot to see that 21_STABLE had far more tests than 22_STABLE/master. Also, note that I had to reinstall one time my 22_STABLE dev server because the first execution ended deleting my admin user in the site, not sure why (perhaps I enabled AMF, not sure). Please, confirm if that's the expected behavior with the patch applied. And I will integrate / reject accordingly to that. TIA and ciao
          Hide
          Jérôme Mouneyrac added a comment - - edited

          In my opinion these "unit tests" are clunky and they should be removed from Moodle. Since we have demo clients, they are less useful. I still run them time to time but I always been doubtful about anyone else running them.

          Otherwise you can give me the error messages and reject this issue. I'll fix the errors.

          Show
          Jérôme Mouneyrac added a comment - - edited In my opinion these "unit tests" are clunky and they should be removed from Moodle. Since we have demo clients, they are less useful. I still run them time to time but I always been doubtful about anyone else running them. Otherwise you can give me the error messages and reject this issue. I'll fix the errors.
          Hide
          Jérôme Mouneyrac added a comment -

          I'm modifying the way to test the other issue depend of this one, so they can be closed.

          Show
          Jérôme Mouneyrac added a comment - I'm modifying the way to test the other issue depend of this one, so they can be closed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, they are "clunky" (whatever it means, lol).

          But we need some programatic (automated) way to verify they are working ok, and those unit tests are the ones "near" to be able to fulfill that requirement.

          Surely in the future, we'll need some sort of proper automated installation + generation + run and test all the protocols/services/functions in a safe environment. And that sounds like something to be done asap once we define out new phpunit-based testing framework.

          But, in the mean time, we need current tests being executed, covering as much as possible, and passing.

          So, if with some little changes, we can incorporate them to the CI tests, the better.

          I'm re-executing them now to report here the current errors I'm getting....

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, they are "clunky" (whatever it means, lol). But we need some programatic (automated) way to verify they are working ok, and those unit tests are the ones "near" to be able to fulfill that requirement. Surely in the future, we'll need some sort of proper automated installation + generation + run and test all the protocols/services/functions in a safe environment. And that sounds like something to be done asap once we define out new phpunit-based testing framework. But, in the mean time, we need current tests being executed, covering as much as possible, and passing. So, if with some little changes, we can incorporate them to the CI tests, the better. I'm re-executing them now to report here the current errors I'm getting....
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For all the executions I've:

          • edited the testwebservice.php, setting one admin-user token, enabled all the protocols and all the functions.
          • set debug developer and enable display errors.
          • the site is one mysql installed site with 1 restored course. Nothing else.

          master (yay passed with new site, was not yesterday):

          1/1 test cases complete: 483 passes, 0 fails and 0 exceptions.

          22_STABLE (better than yesterday against existing site too, but the numbers changed between 1st and 2nd executions):

          1/1 test cases complete: 208 passes, 0 fails and 1 exceptions.
          1/1 test cases complete: 125 passes, 0 fails and 1 exceptions.

          Exception: webservice/simpletest/testwebservice.php / ▶ webservice_test / ▶ testRun
          Unexpected exception of type [Zend_XmlRpc_Client_FaultException] with message [Invalid parameter value detected | DEBUG INFO: users => Invalid parameter value detected: timezone => Invalid parameter value detected: Invalid external api parameter: the value is "98", the server was expecting "timezone" type] in [/Users/stronk7/git_moodle/integration/lib/zend/Zend/XmlRpc/Client.php line 370]
          

          21_STABLE: Numbers also changed between executions

          1/1 test cases complete: 208 passes, 0 fails and 1 exceptions.
          1/1 test cases complete: 145 passes, 0 fails and 1 exceptions.

          (same exception than the one above)

          Shame today errors are different from yesterday ones, grrr.

          Oki, so I'm going to reopen this to see if we can stabilize them somehow. Not looking for perfection, but at least constant results between runs and 100% passing. Also, ideally any missing function should be added if possible.

          For your consideration, feel free to comment/discuss/share... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For all the executions I've: edited the testwebservice.php, setting one admin-user token, enabled all the protocols and all the functions. set debug developer and enable display errors. the site is one mysql installed site with 1 restored course. Nothing else. master (yay passed with new site, was not yesterday): 1/1 test cases complete: 483 passes, 0 fails and 0 exceptions. 22_STABLE (better than yesterday against existing site too, but the numbers changed between 1st and 2nd executions): 1/1 test cases complete: 208 passes, 0 fails and 1 exceptions. 1/1 test cases complete: 125 passes, 0 fails and 1 exceptions. Exception: webservice/simpletest/testwebservice.php / ▶ webservice_test / ▶ testRun Unexpected exception of type [Zend_XmlRpc_Client_FaultException] with message [Invalid parameter value detected | DEBUG INFO: users => Invalid parameter value detected: timezone => Invalid parameter value detected: Invalid external api parameter: the value is "98" , the server was expecting "timezone" type] in [/Users/stronk7/git_moodle/integration/lib/zend/Zend/XmlRpc/Client.php line 370] 21_STABLE: Numbers also changed between executions 1/1 test cases complete: 208 passes, 0 fails and 1 exceptions. 1/1 test cases complete: 145 passes, 0 fails and 1 exceptions. (same exception than the one above) Shame today errors are different from yesterday ones, grrr. Oki, so I'm going to reopen this to see if we can stabilize them somehow. Not looking for perfection, but at least constant results between runs and 100% passing. Also, ideally any missing function should be added if possible. For your consideration, feel free to comment/discuss/share... ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Where did you get this '98' value? What is the function failing? I supposed it was moodle_user_update_users but it is set to 99 and the unit test works passes.

          Show
          Jérôme Mouneyrac added a comment - Where did you get this '98' value? What is the function failing? I supposed it was moodle_user_update_users but it is set to 99 and the unit test works passes.
          Hide
          Jérôme Mouneyrac added a comment -

          I check 98, was the value before the patch. I think you tested without the patch

          Show
          Jérôme Mouneyrac added a comment - I check 98, was the value before the patch. I think you tested without the patch
          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
          Eloy Lafuente (stronk7) added a comment -

          Oh, Jerome, the master branch is now conflicting like crazy because this has not been rebased with last weekly, where MDL-31710 cleanup happened.

          Going to try the stable ones...

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, Jerome, the master branch is now conflicting like crazy because this has not been rebased with last weekly, where MDL-31710 cleanup happened. Going to try the stable ones...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          22_STABLE applies ok, but once the token and all the protocols and functions have been enabled I get:

          Exception: webservice/simpletest/testwebservice.php / ▶ webservice_test / ▶ testRun
          Unexpected exception of type [moodle_exception] with message [Exception with missing
          language string {testdatacustomfieldsalreadyexist} from language file {error}] in
          [/Users/stronk7/git_moodle/integration/webservice/simpletest/testwebservice.php
          line 619]
          line 135 of /webservice/simpletest/testwebservice.php: call to webservice_test->moodle_user_create_users()
          line ... of ...
          
          1/1 test cases complete: 61 passes, 0 fails and 1 exceptions.
          
          Show
          Eloy Lafuente (stronk7) added a comment - 22_STABLE applies ok, but once the token and all the protocols and functions have been enabled I get: Exception: webservice/simpletest/testwebservice.php / ▶ webservice_test / ▶ testRun Unexpected exception of type [moodle_exception] with message [Exception with missing language string {testdatacustomfieldsalreadyexist} from language file {error}] in [/Users/stronk7/git_moodle/integration/webservice/simpletest/testwebservice.php line 619] line 135 of /webservice/simpletest/testwebservice.php: call to webservice_test->moodle_user_create_users() line ... of ... 1/1 test cases complete: 61 passes, 0 fails and 1 exceptions.
          Hide
          Jérôme Mouneyrac added a comment -

          I push back a fixed master

          Show
          Jérôme Mouneyrac added a comment - I push back a fixed master
          Hide
          Jérôme Mouneyrac added a comment -

          Fixed the issue reported in 2.2

          Show
          Jérôme Mouneyrac added a comment - Fixed the issue reported in 2.2
          Hide
          Jérôme Mouneyrac added a comment -

          Fixed in 2.1. Three branches have been rebased. Submitting for integration.

          Show
          Jérôme Mouneyrac added a comment - Fixed in 2.1. Three branches have been rebased. Submitting for integration.
          Hide
          Jérôme Mouneyrac added a comment -

          Note that they all are different commit are they all have different code now.

          Show
          Jérôme Mouneyrac added a comment - Note that they all are different commit are they all have different code now.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Eloy Lafuente (stronk7) added a comment - - edited

          Hoho, I just applied the master branch here, then enabled all the protocols and functions, and set the token to the admin's one.

          And the admin user (id=2) has been deleted from the database completely and no test is passing anymore (as far as all them look in the user table for id=2, the current user I'm logged as).

          Perhaps we should finally move this to won't fix and try the phpunit alternative that already offers one 100% isolated environment for execution and other facilities?

          Uhm.. I'm going to keep this open for further discussion. Moving to phpunit will mean that we lose the 2.0, 2.1 and 2.2 tests.... so I've divided feelings...

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hoho, I just applied the master branch here, then enabled all the protocols and functions, and set the token to the admin's one. And the admin user (id=2) has been deleted from the database completely and no test is passing anymore (as far as all them look in the user table for id=2, the current user I'm logged as). Perhaps we should finally move this to won't fix and try the phpunit alternative that already offers one 100% isolated environment for execution and other facilities? Uhm.. I'm going to keep this open for further discussion. Moving to phpunit will mean that we lose the 2.0, 2.1 and 2.2 tests.... so I've divided feelings...
          Hide
          Jérôme Mouneyrac added a comment -

          If I were you I'll integrate it as this version is better that the current one However I agree that we should stop working on it and go to phpunit as it's the new test framework for Moodle.

          Show
          Jérôme Mouneyrac added a comment - If I were you I'll integrate it as this version is better that the current one However I agree that we should stop working on it and go to phpunit as it's the new test framework for Moodle.
          Hide
          Jérôme Mouneyrac added a comment -

          Resubmitting to see if you prefer to keep the current badly broken or this one which is way better (the issue you are mentioning are anyway in the current one )

          Show
          Jérôme Mouneyrac added a comment - Resubmitting to see if you prefer to keep the current badly broken or this one which is way better (the issue you are mentioning are anyway in the current one )
          Hide
          Jérôme Mouneyrac added a comment -

          After talking with Eloy we decided that we will not fix this "clunky" unit test file. The reason being that we are moving to PHPunit in 2.3 and we will "port" these tests to PHPunit. I will move the "best" working version of these simpletests to https://github.com/mouneyrac/local_wstesting

          I will keep this local plugin simpletest working till we have the PHPunit web service replacement ready. I'll also write a Moodledocs soon for the ws unit test development roadmap. I'll mention the doc url here one it exists.

          Thanks Eloy and Rosie, even thought we didn't integrate these changes I appreciated your help and efforts

          Show
          Jérôme Mouneyrac added a comment - After talking with Eloy we decided that we will not fix this "clunky" unit test file. The reason being that we are moving to PHPunit in 2.3 and we will "port" these tests to PHPunit. I will move the "best" working version of these simpletests to https://github.com/mouneyrac/local_wstesting I will keep this local plugin simpletest working till we have the PHPunit web service replacement ready. I'll also write a Moodledocs soon for the ws unit test development roadmap. I'll mention the doc url here one it exists. Thanks Eloy and Rosie, even thought we didn't integrate these changes I appreciated your help and efforts

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: