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

message/tests/externallib_test.php is broken

    Details

      Description

      It uses message_send which is prohibited with transactions.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            1) core_message_external_testcase::test_send_instant_messages
            dml_transaction_exception: Database transaction error (This code can not be excecuted in transaction)

            /Users/danp/git/integration/lib/dml/moodle_database.php:2117
            /Users/danp/git/integration/lib/messagelib.php:61
            /Users/danp/git/integration/message/lib.php:2021
            /Users/danp/git/integration/message/externallib.php:147
            /Users/danp/git/integration/message/tests/externallib_test.php:65
            /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

            To re-run:
            vendor/bin/phpunit core_message_external_testcase message/tests/externallib_test.php

            Show
            poltawski Dan Poltawski added a comment - 1) core_message_external_testcase::test_send_instant_messages dml_transaction_exception: Database transaction error (This code can not be excecuted in transaction) /Users/danp/git/integration/lib/dml/moodle_database.php:2117 /Users/danp/git/integration/lib/messagelib.php:61 /Users/danp/git/integration/message/lib.php:2021 /Users/danp/git/integration/message/externallib.php:147 /Users/danp/git/integration/message/tests/externallib_test.php:65 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit core_message_external_testcase message/tests/externallib_test.php
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            I created a different issue (MDL-37437) to go through all external unit test and add them to the automatic PHPunit run.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited I created a different issue ( MDL-37437 ) to go through all external unit test and add them to the automatic PHPunit run.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            ping peer-review as in been in progress for some days. Don't hesisate to throw it back in development status if you don't have time. Thanks.

            Show
            jerome Jérôme Mouneyrac added a comment - ping peer-review as in been in progress for some days. Don't hesisate to throw it back in development status if you don't have time. Thanks.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Sorry for the delay Jerome,

            Patch looks good, please add testing phpunit on pgsql as this is not reproducible on mysql.

            [y] Syntax
            [y] Output
            [y] Whitespace
            [-] Language
            [-] Databases
            [y] Testing
            [-] Security
            [-] Documentation
            [y] Git
            [y] Sanity check

            Show
            rajeshtaneja Rajesh Taneja added a comment - Sorry for the delay Jerome, Patch looks good, please add testing phpunit on pgsql as this is not reproducible on mysql. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
            Hide
            jerome Jérôme Mouneyrac added a comment -

            No worries Raj. Thanks finding out about mysql/postgres, it does explain why I didn't notice, I used mysql at the time I wrote the PHPunit test. So I did tested it Dan

            Show
            jerome Jérôme Mouneyrac added a comment - No worries Raj. Thanks finding out about mysql/postgres, it does explain why I didn't notice, I used mysql at the time I wrote the PHPunit test. So I did tested it Dan
            Hide
            poltawski Dan Poltawski added a comment -

            That doesn't sound right, are you using myiasm Raj?

            Show
            poltawski Dan Poltawski added a comment - That doesn't sound right, are you using myiasm Raj?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Note that not all DBs use the same mechanism to "reset" the testing site at the begin of each test case.

            It sounds to me that some DBs use transactions for that, so one simple rollback will reset everything, while others perform a sort of delete + re-insert.

            Perhaps Postgres is using the rollback way, meaning it starts a transaction, leading to the error when transactions_forbidden() is used in the middle of a test.

            Just in case it helps to clarify why it may be breaking only for postgres. Perhaps the solution is to hack the transactions_forbidden when running tests. I'd ask Petr, sure he knows better.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Note that not all DBs use the same mechanism to "reset" the testing site at the begin of each test case. It sounds to me that some DBs use transactions for that, so one simple rollback will reset everything, while others perform a sort of delete + re-insert. Perhaps Postgres is using the rollback way, meaning it starts a transaction, leading to the error when transactions_forbidden() is used in the middle of a test. Just in case it helps to clarify why it may be breaking only for postgres. Perhaps the solution is to hack the transactions_forbidden when running tests. I'd ask Petr, sure he knows better. Ciao
            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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi, Petr just pointed to me that, if the code being tested is using transactions... then the test case must use this:

            $this->preventResetByRollback();

            to avoid it to use transactions for resetting.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi, Petr just pointed to me that, if the code being tested is using transactions... then the test case must use this: $this->preventResetByRollback(); to avoid it to use transactions for resetting. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (24 & master), thanks!

            Note: At the end I've ignored your commit and introduces alternative one. Yours was conflicting (whole file), not sure why, really and also I don't think disabling reset was a good idea (we needed to ensure that reset happens, just not using db transactions).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks! Note: At the end I've ignored your commit and introduces alternative one. Yours was conflicting (whole file), not sure why, really and also I don't think disabling reset was a good idea (we needed to ensure that reset happens, just not using db transactions).
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing, CI server running PG tell it's ok now.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing, CI server running PG tell it's ok now.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

            (and won't be revisiting it unless some regression is found)

            Thanks and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Mar/13