#1705748: Convert simpletest settings to configuration system removes the usage of variable_get for 'simpletest_clear_results' and 'simpletest_verbose', leading to all tests showing 0 passes, since the tables are cleared.


Status:Active» Needs review
new767 bytes

See attached.

Project:Drupal.org Testbots» Project Issue File Review
Version:» 6.x-2.x-dev
Component:malfunctioning testbot» Code


Category:bug» task
Status:Needs review» Needs work

This needs to be done in a non-d8 code way. We are running in a d6 module here. We just need to put the two variables in a yml file somewhere that the system picks them up.

I am not familiar with d8 config system.

Webchick says $conf in settings.php still works, possibility. Not sure if that is going to remain long term (betting not).

Otherwise we need to mock some of the code to generate setting package directly. Also it might be a possibility to call the config code directly if it isn't drupal dependent...just to generate file.

Why do those two variables affect the test results in the first place though?

They're disabling verbose output and clearing of results.

However, run-tests.sh does not use verbose output by default, and run-tests.sh should also clean up test results.

...in any case, it seems a little weird that these two variables have any impact on the test runner...?

It usually holds onto test results long enough to report something more than 0 passes.

The test results from #1705748-37: Convert simpletest settings to configuration system demonstrate what I mean.

catch confirmed that $conf is the only way to do this, and that it's not going away anytime soon. If it does, it will have a replacement.

So the patch in #1705748-39: Convert simpletest settings to configuration system would have to be orchestrated with attached patch for PIFR.

Status:Needs work» Needs review

Ok flag patch got committed. My fix for catching this type of issue in the future is at #873496: Bogus test results PASSED: [[SimpleTest]]: [MySQL] 0 passes. so now we just need to test out the pifr patch with the change to simpletest config.

new2.41 KB

We should not bother to set the variables in d8 since they do not work and might as well use the standard way of checking for support.

Why don't we backport #1719530: Add --keep-results flag to run-tests.sh to D7 and also apply it to D6's Simpletest, and remove this hack entirely? :)

It is unnecessary since they are stable and work, but sure it is better. Although a hack is a bit further than I would take it. Lets get the actual working patch in d8 first :)

Status:Needs review» Fixed

Committed current patch so we don't hold things up, we can commit another one if we do all the backporting.

Status:Fixed» Postponed

+++ b/review/simpletest/pifr_simpletest.client.inc
@@ -128,10 +130,17 @@ class pifr_client_review_pifr_simpletest extends pifr_client_review_pifr_drupal
+    if ($this->arguments['drupal.core.version'] >= 8) {
+      $command .= ' --keep-results';
+    }

The idea of #9 was to avoid further patches to PIFR by simply checking whether run-tests.sh supports the --keep-results option, instead of hard-coding a core major version condition.

With the committed code, the proper status here is postponed, since we should adjust the version conditions when it got backported.

The reason I changed it was to be consistent with other checks. Seems like if we are going to change the way it works we should do so, but not just for one case. Long term it seems a bit expensive to scan an entire file to determine whether or not to use the flag. It is definitely more adaptive and for that reason perhaps it is better during the interim of the switch.

btw, the testbot is not properly picking up the (non-existing) --verbose flag.

You can see in all more recent test review results that there are tons of "verbose message" debug output rows in the results (by expanding the review results for many test cases).

I'm relatively sure that this is an unintended/unexpected artifact of the alternative approach that has been chosen here.