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

          Attachments

            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