Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Messages, Web Services
    • Labels:
    • Rank:
      43556

      Description

      write PHPunit tests for message/externallib.php

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Sent to Rosie for peer-review.

          Show
          Jérôme Mouneyrac added a comment - Sent to Rosie for peer-review.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          Some feedback for the patch:

          1. Line 33-39 could be removed. It is a duplication of the above comments.
          2. @category value should probably assign as phpunit instead of external
          3. it might be better to remove the setUp() and place the global $CFG and require_once on top of the page.
          4. Line 60, could probably be moved to the top of page

          The rest looks good.

          Show
          Rossiani Wijaya added a comment - Hi Jerome, Some feedback for the patch: Line 33-39 could be removed. It is a duplication of the above comments. @category value should probably assign as phpunit instead of external it might be better to remove the setUp() and place the global $CFG and require_once on top of the page. Line 60, could probably be moved to the top of page The rest looks good.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Rosie. I changed 1) and 3). I kept 2) because I'd like people who write external file to see these specific phpunit test (as they extend a phpunit test for external file). I kept 4) because it's specific to this function and I suppose it will be/is possible to run only one test function.

          Show
          Jérôme Mouneyrac added a comment - Thanks Rosie. I changed 1) and 3). I kept 2) because I'd like people who write external file to see these specific phpunit test (as they extend a phpunit test for external file). I kept 4) because it's specific to this function and I suppose it will be/is possible to run only one test function.
          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
          Aparup Banerjee added a comment -

          thanks, thats integrated into master.

          Show
          Aparup Banerjee added a comment - thanks, thats integrated into master.
          Hide
          Tim Barker added a comment -

          PHPUnit tests are currently tested on MySQL and PgSQL automatically. They pass, we don't have the infrastructure to test against every DB driver atm BUT, we are in an advanced stage of provisioning these environments and I may be able to get something up and running on MSSQL in the next fortnight.

          Show
          Tim Barker added a comment - PHPUnit tests are currently tested on MySQL and PgSQL automatically. They pass, we don't have the infrastructure to test against every DB driver atm BUT, we are in an advanced stage of provisioning these environments and I may be able to get something up and running on MSSQL in the next fortnight.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!
          Hide
          Dan Poltawski added a comment -

          Was this ever tested during development!? It looks to me like it was not as the code its calling doesn't work inside transactions.

          It also wasn't tested automatically because the directory was added but not to the phpunit.xml.

          See MDL-37086

          Show
          Dan Poltawski added a comment - Was this ever tested during development!? It looks to me like it was not as the code its calling doesn't work inside transactions. It also wasn't tested automatically because the directory was added but not to the phpunit.xml. See MDL-37086
          Hide
          Jérôme Mouneyrac added a comment -

          I found this issue fixing MDL-37354. You would have asked me if I tested it few days ago, I would have said "yes for sure, I test everything I push". Now seeing this error I have a doubt...

          For the phpunit.xml, I didn't know about it. I'll check it, create an issue and fix it. Thanks Dan.

          Show
          Jérôme Mouneyrac added a comment - I found this issue fixing MDL-37354 . You would have asked me if I tested it few days ago, I would have said "yes for sure, I test everything I push". Now seeing this error I have a doubt... For the phpunit.xml, I didn't know about it. I'll check it, create an issue and fix it. Thanks Dan.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: