Details

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

      Description

      write PHPunit tests for message/externallib.php

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              jerome Jérôme Mouneyrac added a comment -

              Sent to Rosie for peer-review.

              Show
              jerome Jérôme Mouneyrac added a comment - Sent to Rosie for peer-review.
              Hide
              rwijaya 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
              rwijaya 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
              jerome 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
              jerome 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
              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
              nebgor Aparup Banerjee added a comment -

              thanks, thats integrated into master.

              Show
              nebgor Aparup Banerjee added a comment - thanks, thats integrated into master.
              Hide
              timb 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
              timb 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
              stronk7 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
              stronk7 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
              poltawski 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
              poltawski 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
              jerome 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
              jerome 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:
                    Fix Release Date:
                    3/Dec/12