Sent to Rosie for peer-review.

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.

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.

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

thanks, thats integrated into master.

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.

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!

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

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.