FMCorz / mdk

Moodle Development Kit. A collection of tools meant to make developers' lives easier.
GNU General Public License v3.0
85 stars 47 forks source link

Automatically build distributed phpunit.xml files for each component #170

Closed mudrd8mz closed 6 years ago

mudrd8mz commented 6 years ago

I often make use of the buildcomponentconfigs feature allowing me to run all unit tests for the given component, such as

vendor/bin/phpunit -c mod/workshop/

To be able to do so, I have to manually run admin/tool/phpunit/cli/util.php --buildcomponentconfigs every time. It makes sense to let mdk do it automatically.

Adding this feature was already discussed with @FMCorz at https://moodle.org/local/chatlogs/index.php?conversationid=19150#c605808 who has alternate way to achieve the same. However, I fail to remember that one. Also, there are cases when I simply run the phpunit manually without mdk.

So I still think it is worth considering to add this as an option, enabled by default as it does not hurt.

Further references: https://moodle.org/local/chatlogs/index.php?q=buildcomponentconfigs

FMCorz commented 6 years ago

Thanks!

FMCorz commented 6 years ago

I forgot to ask, could you explain why using strtobool? That argument should be a boolean already. If not, then there is a problem in the config API, or the config file. If strtobool is not needed, could you please submit a patch removing it? Thanks!

mudrd8mz commented 6 years ago

Plain if C.get('...') did not work for me and the branch was always evaluated even if the value was set to false. A bit of debugging revealed that C.get() returns the type unicode and Python interpreted that string 'false' as truthy value. Only using strtobool actually converted values like false on n etc to boolean and evaluation worked.

FMCorz commented 6 years ago

I suspect that there is a problem with your config file.

$ mdk config show phpunit
phpunit.buildcomponentconfigs: True

Boolean values in JSON should not be translated to string. You must have an overridden config that is set as the string "true" rather than the boolean. Commonly:

$ mdk config set phpunit.buildcomponentconfigs false
$ mdk config show phpunit
phpunit.buildcomponentconfigs: false
$ mdk config set phpunit.buildcomponentconfigs b:false
$ mdk config show phpunit
phpunit.buildcomponentconfigs: False

Note the capital letter which denotes the boolean vs the string.

FMCorz commented 6 years ago

I'm realising that this (prefixing the value) is not documented in --help, and probably could do with a more predictable method such as setting a flag --bool or --integer when calling mdk config set instead of my very cryptic method...

mudrd8mz commented 6 years ago

I used what I saw in other places in config-dist.json and the plain

"buildcomponentconfigs": false

just did not work, the branch was always executed.

FMCorz commented 6 years ago

As discussed, I prefer trusting the typing coming from the configuration file, and not converting values from string to what they could be. I recognise that the config command isn't that friendly when it comes to setting values, so in the commit 540260b, I implemented:

mudrd8mz commented 6 years ago

Thanks Fred. I like the implemented solution, well done.