#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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
767 bytes

See attached.

tim.plunkett’s picture

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

Moving

boombatower’s picture

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.

boombatower’s picture

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.

sun’s picture

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...?

tim.plunkett’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

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.

sun’s picture

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

sun’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

boombatower’s picture

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.

boombatower’s picture

FileSize
2.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.

sun’s picture

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? :)

boombatower’s picture

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 :)

boombatower’s picture

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.

sun’s picture

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.

boombatower’s picture

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.

sun’s picture

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.

boombatower’s picture