Moodle
  1. Moodle
  2. MDL-37086

message/tests/externallib_test.php is broken

    Details

    • Rank:
      46643

      Description

      It uses message_send which is prohibited with transactions.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - That doesn't sound right, are you using myiasm Raj?
          Hide
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Passing, CI server running PG tell it's ok now.
          Hide
          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
          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: