Moodle

PHPunit test: message/externallib.php

Details

  • Type: Sub-task Sub-task
  • Status: Closed 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

Issue Links

caused a regression

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-37086 message/tests/externallib_test.php is broken

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Aparup Banerjee added a comment -

thanks, thats integrated into master.

Show
Aparup Banerjee added a comment - thanks, thats integrated into master.
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: