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:

      Description

      write PHPunit tests for message/externallib.php

        Gliffy Diagrams

          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: