Convert simpletest settings to configuration system

Tagging.

Awesome! :)

+++ b/core/modules/simpletest/config/simpletest.settings.yml
@@ -0,0 +1,7 @@
+clear_results: 1

In YAML, all values are strings by default, so strings do not need any special treatment.

However, integers, numbers and any other special values need to be wrapped in single-quotes ('').

+++ b/core/modules/simpletest/config/simpletest.settings.yml
@@ -0,0 +1,7 @@
+httpauth.password:
+httpauth.username:

Likewise, empty strings need to be explicitly specified as such; i.e.: ''

+++ b/core/modules/simpletest/config/simpletest.settings.yml
@@ -0,0 +1,7 @@
+parent_profile: 0

This value should be an empty string.

+++ b/core/modules/simpletest/simpletest.install
@@ -171,12 +171,25 @@ function simpletest_uninstall() {
   // Remove settings variables.
-  variable_del('simpletest_httpauth_method');
-  variable_del('simpletest_httpauth_username');
-  variable_del('simpletest_httpauth_password');
-  variable_del('simpletest_clear_results');
-  variable_del('simpletest_verbose');
+  config('simpletest.settings')
+    ->delete('clear_results')
+    ->delete('httpauth.method')
+    ->delete('httpauth.password')
+    ->delete('httpauth.username')
+    ->delete('maximum_redirects')
+    ->delete('parent_profile')
+    ->delete('verbose');

All configuration objects of a module are automatically deleted upon uninstallation. These lines need to be removed entirely instead. :)

+++ b/core/modules/simpletest/simpletest.install
@@ -171,12 +171,25 @@ function simpletest_uninstall() {
+function simpletest_update_8000() {
+   update_variables_to_config('simpletest.settings', array('simpletest_clear_results' => 'clear_results'));
+   update_variables_to_config('simpletest.settings', array('simpletest_httpauth_password' => 'httpauth.method'));
+   update_variables_to_config('simpletest.settings', array('simpletest_httpauth_password' => 'httpauth.password'));
+   update_variables_to_config('simpletest.settings', array('simpletest_httpauth_username' => 'httpauth.username'));
+   update_variables_to_config('simpletest.settings', array('simpletest_maximum_redirects' => 'maximum_redirects'));
+   update_variables_to_config('simpletest.settings', array('simpletest_parent_profile' => 'parent_profile'));
+   update_variables_to_config('simpletest.settings', array('simpletest_verbose' => 'verbose'));
+}

ugh :) Please check the patch in http://drupal.org/node/1496458#comment-6285700 for how this should look like ;)

+++ b/core/modules/simpletest/simpletest.pages.inc
@@ -436,6 +436,7 @@ function simpletest_result_status_image($status) {
+  $simpletest_config = config('simpletest.settings');

The variable can be renamed to just $config, since this is the settings form of Simpletest module, so there's naturally no other $config involved.

Lastly, it's possible that we might miss some instances in core/scripts/run-tests.sh -- or did you check that already?

+++ b/core/modules/simpletest/config/simpletest.settings.yml
@@ -1,7 +1,7 @@
+httpauth.method: '1'
+httpauth.password: ''
+httpauth.username: ''

oh, sorry, I completely overlooked a blatant mistake here -- every dot in a key is transformed into a nested array key in the .yml file. I.e., this needs to look like this:

httpauth:
    method: '1'
    password: ''
    username: ''

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -483,10 +483,11 @@ abstract class TestBase {
-    $this->httpauth_method = variable_get('simpletest_httpauth_method', CURLAUTH_BASIC);
...
+    $this->httpauth_method = $simpletest_config->get('httpauth.method');

We most likely need to cast the value of httpauth.method into an integer like this:

$this->httpauth_method = (int) $config->get(...);

That is, because the value 1 refers to the cURL constant value CURLAUTH_BASIC, and cURL won't know what to do with a '1' (string).

+++ b/core/modules/simpletest/simpletest.install
@@ -170,13 +170,18 @@ function simpletest_uninstall() {
+function simpletest_update_8000() {

We're still missing the phpDoc for this function :)

+++ b/core/modules/simpletest/simpletest.install
@@ -170,13 +170,18 @@ function simpletest_uninstall() {
+  update_variables_to_config('system.maintenance', array(

The variables are migrated into the wrong config object! ;)

+++ b/core/modules/simpletest/simpletest.install
@@ -170,13 +170,18 @@ function simpletest_uninstall() {
+    'simpletest_httpauth_password' => 'httpauth.method',

Variable mismatch! :)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php
@@ -76,7 +76,7 @@ class SimpleTestTest extends WebTestBase {
-      variable_set('simpletest_maximum_redirects', 1);
+      config('simpletest.settings')->set('maximum_redirects', 1);

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -625,7 +625,7 @@ abstract class WebTestBase extends TestBase {
-    variable_set('simpletest_parent_profile', $this->originalProfile);
+    config('simpletest.settings')->set('parent_profile', $this->originalProfile);

We're missing a ->save() here. :)

I don't see what's wrong with this patch.

Most likely, the test failures are caused by the change to drupal_system_listing(), which in turn means that also this patch is blocked on #1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc - which will hopefully land very soon.

#13: config-simpletest.12.patch queued for re-testing.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -483,13 +483,17 @@ abstract class TestBase {
+    $simpletest_config = config('simpletest.settings');
...
+    $this->maximumRedirects = config('simpletest.settings')->get('maximum_redirects');

We already have that config object in $simpletest_config here? :)

Couple of minor nitpicks...

+++ b/core/modules/simpletest/simpletest.pages.incundefined
@@ -436,6 +436,7 @@ function simpletest_result_status_image($status) {
  * @see simpletest_settings_form_validate()
  */
 function simpletest_settings_form($form, &$form_state) {
+  $config = config('simpletest.settings');

Need to add @see simpletest_settings_form_submit()

+++ b/core/modules/simpletest/simpletest.pages.incundefined
@@ -491,7 +492,22 @@ function simpletest_settings_form($form, &$form_state) {
+/**
+ * Form submission handler for simpletest_settings_form().
+ *
+ * @ingroup forms
+ */
+function simpletest_settings_form_submit($form, &$form_state) {

The @ingroup forms here is unnecessary. It's for functions that produce form arrays not submission handlers

Crosspost...

Yay! :)

This patch is RTBC...

...however, the additionally required fix for maximumRedirects also applies to the verbose setting, which makes this super-nice patch blocked by #1611430: Verbose debug output in unit tests relies on the database

That is, because the config() in simpletest_verbose() will effectively try to read the verbose setting from the simpletest.settings config file of the "site under test" --- but that environment might not even have Simpletest module installed.

Essentially, this requires the very same change as for maximumRedirects - but that ain't possible, as long as TestBase calls into a completely detached procedural simpletest_verbose() function, which is not able to access the test class' properties anymore.

#24: config-simpletest.24.patch queued for re-testing.

Awesome!!! :)

Minor style fix attached. Per http://drupal.org/node/1354#hookimpl, update hooks get the imperative (the summaries are user-facing).

Committed to 8.x. Thanks.

This issue causes #1717868: PIFR uses variable_get() but needs config() in Drupal 8 (note how the patches above have 0 passes). I left a message asking webchick to git revert 490386f and reopen this issue if a solution for that other issue is not straightforward, because no tests are running for any projects since this was committed.

If that issue is resolved, we can set the status of this one back without reverting.

Reverted this patch for now using the command in #34. Reverting status.

Almost.

On a hunch, let's see what these results are.

Forgot an interdiff, here's a cheap one.
The results prove my theory, that these simpletest variables are responsible for the results persisting long enough for testbot to display results.

Attached patch requires PIFR to invoke simpletest with --keep-results

Split this out into #1719530: Add --keep-results flag to run-tests.sh so it can be committed without breaking core.

#31: simpletest-1705748-31.patch queued for re-testing.

Testing out to see if a patched testbot will detect this fun edge-case since humans seem to miss the 0.

http://drupal.org/node/873496#comment-6332732

Check out #31 vs #28! Fix http://drupal.org/node/873496#comment-6332868. Looking for feedback on the repercussions.

Now that #1719530: Add --keep-results flag to run-tests.sh made it in, I have patch a pifr bot in advance, and rerolled this patch.

Note that I didn't have the pifr patch to catch the 0 results applied...only the patch to add flag when calling run-tests.sh...so it seems either run-tests.sh patch does not work of pifr patch does not.

#45: 1705748-simpletest-config.patch queued for re-testing.

EDIT: flag patch fix was committed.
EDIT: Looks like someone broke my pifr code that allows me to disable a bot running a test so it won't ask for another...i'll have to retest again.
EDIT: tested on patch client and you will see my patch returned proper results.

EDIT: Hold off on committing until we can push to all testbots. #1723044: I no longer have access to update testbots

#45: 1705748-simpletest-config.patch queued for re-testing.

It ran run-tests.sh with new flag properly, from log:

/usr/bin/php ./core/scripts/run-tests.sh --concurrency 8 --php /usr/bin/php --url 'http://drupaltestbot659-mysql/checkout' --keep-results --all

So looks like we should be able to commit this safely now. Along with that deployment is the one to catch crud like this in the future so we hopefully can stop having these sorts of rare issues.

Thanks.

Actually, before we jump too fast. Based on your comment http://drupal.org/node/1717868#comment-6422150 I looked into the --verbose flag which I remember existing, but not to replace the simpletest_verbose variable.

  --verbose   Output detailed assertion messages in addition to summary.

Which is intended to spit out the individual row-by-row messages (assertions) in the output of the run-tests.sh script. Which is quite different from the simpletest_verbose variable which was to control the verbose page capturing functionality in DrupalWebTestCase.

We probably want to split these out and rename one. May also want to default to false on the command line? dunno.

I think --verbose flag as it stand now works the way that is most consistent with other scripts and makes sense. Perhaps we rename simpletest_verbose variable to 'capture' or something along those lines to 'capture browser shot' etc. I don't like those names just providing a thought basis.

If we default cli to false then using the cli then might do.

--print-details --verbose

--print-details to print out assertion messages
--print-results
--results
--details
,etc

@boombatower: Let's move that discussion on --verbose into a separate issue. This patch here doesn't actually influence that, as it doesn't touch run-tests.sh and so on at all.

+++ b/core/modules/simpletest/config/simpletest.settings.yml
@@ -0,0 +1,8 @@
+maximum_redirects: '5'

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -541,13 +543,16 @@ abstract class TestBase {
+    // Maximum redirects setting for the simpletest browser.
+    $this->maximumRedirects = $simpletest_config->get('maximum_redirects');

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php
@@ -84,7 +84,7 @@ class SimpleTestTest extends WebTestBase {
+      $this->maximumRedirects = 1;

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -911,7 +911,7 @@ abstract class WebTestBase extends TestBase {
     // TODO: Remove this for Drupal 8, since fixed in curl 7.20.0.
...
+    if (in_array($status, array(300, 301, 302, 303, 305, 307)) && $this->redirect_count < $this->maximumRedirects) {

+++ b/core/modules/simpletest/simpletest.install
@@ -170,13 +170,21 @@ function simpletest_uninstall() {
+    'simpletest_maximum_redirects' => 'maximum_redirects',

I wonder whether maximum redirects is actually supposed to be configuration...?

For one, there's at least one test that overrides it, so it appears to be test-case-specific setting/property.

The only other usage is within WebTestBase's internal browser.

In turn, this means that you cannot actually configure this... if you'd configure it to e.g. 0, then surely some tests will fail, because the internal browser would not follow redirects anymore.

Even more interesting is the @todo about it being potentially obsolete for D8, since cURL itself was fixed.

So I guess we want to remove this from the configuration and just turn it into a regular class property.

+++ b/core/modules/simpletest/config/simpletest.settings.yml
@@ -0,0 +1,8 @@
+parent_profile: ''

+++ b/core/modules/simpletest/simpletest.install
@@ -170,13 +170,21 @@ function simpletest_uninstall() {
+function simpletest_update_8000() {
...
+    'simpletest_parent_profile' => 'parent_profile',

This variable actually does not exist on a regular Drupal site, but only within a test run.

Let's delete it from the default config file and also from the update function.

Additionally, let's make sure to remove the stop-gap fix to run-tests.sh that's going to be added in #1719530-15: Add --keep-results flag to run-tests.sh

When I referred to flag it was from your comment in http://drupal.org/node/1717868#comment-6422150, which would seem to imply the testbot should be using the --verbose flag, yet then you had "(non-existing)" which I am not sure if that referred to not adding one for the old setting or what since we do have a flag it just doesn't related to what is being changed.

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

Since we have the temporary hack from the other issue placing the --verbose discussion in another issue is fine, but without that it definitely needed to be solved in this issue along with the --keep-results shift.

Created #1774002: Correct mistake introduced in simpletest cmi conversion by introducing --verbose-browser flag

Adjusted for #53 and also removed the stop-gap fix from run-tests.sh, which is no longer needed with this patch.

Why remove the stop gap measure now since it will simple return to being enabled on the testbot until we resolve and commit #1774002: Correct mistake introduced in simpletest cmi conversion by introducing --verbose-browser flag which seemed to be your preference to split into another issue?

@boombatower: The testbot already adheres to the converted configuration, as you can see in the test results for the patch in #56: http://qa.drupal.org/pifr/test/332313

The separate --verbose-browser option (as proposed in #1774002: Correct mistake introduced in simpletest cmi conversion by introducing --verbose-browser flag) is not relevant for this conversion issue.

Right; the workaround was for these settings not being converted to CMI yet; I don't think it has bearing on #1774002: Correct mistake introduced in simpletest cmi conversion by introducing --verbose-browser flag.

Ok, here we go. Second time's a charm? :)

Committed and pushed to 8.x. Thanks!

I think the yml file was not committed.

*blush*

Committed/pushed to 8.x. Thanks. :P

so yea, I tested/committed/wrote the stuff to make the testbot "convert to cmi" it isn't convert it to cmi...we decided to change to flags instead of setting the configuration directly like testbot did before... unfortunately we did not add a flag to turn off verbose test output and the default remains enabled...so the testbot should now be testing with verbose browser output.

Which is why it seemed logical to either a) include the --verbrose-browser flag in this patch, or before this patch (like with --keep-results), or b) leave the temporary hack in. Whereas now we are just back to a broken implementation.

The patch for adding --keep-results http://drupal.org/node/1719530#comment-6326150 also subtly change simpletest_verbose to be set from --verbose flag and failed to update the documentation. So given that I looked at the code before, documentation, and intentent this does not behave the way it was documented. We can resolve the documentation in the other issue for --verbose-browser if we choose to change or just fix doc.

So this issue is fine due to the changes made in --keep-results (doesn't just add keep results flag).