search

jsxgraph / moodle-filter_jsxgraph

moodle plug-in for JSXGraph
https://jsxgraph.org
2 stars 6 forks source link

Problem with unit test of plugin manager #26

Closed pmaneggia closed 1 year ago

pmaneggia commented 2 years ago

Hi! When running the unit test of the plugin manager (for example --filter {test_teardown_works_precheck}) (I tried in a vanilla Moodle 3.11.6 with no other non core plugin but this one installed) I get:

`There was 1 error:

1) core_plugin_manager_testcase::test_teardown_works_precheck Warning: unexpected database modification, resetting DB state

/var/www/html/lib/phpunit/classes/util.php:300 /var/www/html/lib/phpunit/classes/advanced_testcase.php:534 /var/www/html/lib/phpunit/classes/advanced_testcase.php:132 phpvfscomposer:///var/www/html/vendor/phpunit/phpunit/phpunit:97

ERRORS! Tests: 1, Assertions: 2, Errors: 1.`

So far I could not pin down why this happens, as the plugin itself does not modify the database at all, but I point this out in case some one else has the same problem.

andreas-web commented 2 years ago

Indeed! That would be very interesting to know. Maybe someone else will comment here who has the same problem. I can't figure out either where that should come from, since our filter, as you said, doesn't change anything in the database.

PM84 commented 2 years ago

The problem is the try-catch block in version.php. This is bad practice, as version.php should not contain functional code.

Such changes should be put in an upgrade.php for example. In my PR #27 I did it this way too.

andreas-web commented 2 years ago

Hi!

Thanks for the hint. The problem is that we want to make the $plugin->release attribute available elsewhere in the filter WITHOUT having to enter the release value twice. That's why the try-catch block was built in.

Your pull request would mean changing the value twice: once in version.php and once in upgrade.php.

Maybe you have another idea how to pass the value of $plugin->release directly to other places (e.g. in filter.php or settings.php) or get it from the database. If not, we will unfortunately have to remove the block again in the next version.

Best regards, Andreas

andreas-web commented 1 year ago

Fixed by 559cf2e