Convert _form_test_* functions to classes

Here's a patch that converts the first 3 on the list - needs testing.

New patch - fixing hidden: false in module .info.yml file

Diff file for the 2 patches submitted today

This is looking good. Here's some minor changes:

1. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestCheckboxForm.php
   @@ -0,0 +1,106 @@
   +      '#value' => t('Submit')
   
   +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestDisabledElementsForm.php
   @@ -0,0 +1,239 @@
   +      '#value' => t('Submit'),
   
   +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestInputForgery.php
   @@ -0,0 +1,60 @@
   +      '#value' => t('Submit'),
   

   change to $this->t(

2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestDisabledElementsForm.php
   @@ -0,0 +1,239 @@
   +    $response = new JsonResponse($form_state['values']);
   +
   +    $response->send();
   +
   +    exit;
   
   +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestInputForgery.php
   @@ -0,0 +1,60 @@
   +    $response = new JsonResponse($form_state['values']);
   +
   +    $response->send();
   +
   +    exit;
   

   Just return the JsonResponse. Don't send/exit.

addressing my comments in #7.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

There is no code removed from the patch ... this feels wrong.

1. +++ w/core/modules/system/tests/modules/form_test/form_test.module
   @@ -221,15 +214,7 @@ function form_test_menu() {
   -  $items['form-test/input-forgery'] = array(
   +  $items['form-test/input-forgery-old'] = array(
   

   This change is confusing.

2. +++ w/core/modules/system/tests/modules/form_test/form_test.routing.yml
   @@ -67,3 +67,24 @@ form_test.autocomplete_2:
   +
   +form_test.checkbox:
   +  pattern: '/form-test/checkbox'
   +  defaults:
   +    _form: '\Drupal\form_test\FormTestCheckboxForm'
   +  requirements:
   +    _access: 'TRUE'
   +
   +form_test.disabled_elements:
   +  pattern: '/form-test/disabled-elements'
   +  defaults:
   +    _form: '\Drupal\form_test\FormTestDisabledElementsForm'
   +  requirements:
   +    _access: 'TRUE'
   +
   +form_test.input_forgery:
   +  pattern: '/form-test/input-forgery'
   +  defaults:
   +    _form: '\Drupal\form_test\FormTestInputForgery'
   +  requirements:
   +    _access: 'TRUE'
   

   We maybe should put title on there?

I didn't catch that in my review. Lets hack some code away! Titles are restored to the routes as well, although I don't think there very meaningful (they're all "Form test".

Patch re-rolled.

Patch re-rolled.

Uploading another re-rolled patch

simple reroll

#18 was not helpful without further explaination or even an interdiff. It's quite hard to read from just diffing the patches, but mostly it is converting some more forms, so that's good.

But it also restored use of _form_test_submit_values_json() for no aparent reason and dropped the conversion of _form_test_disabled_elements() (which I agree with, since it looks like that test needs more serious changes)

In #7 disasm said that FormTestInputForgeryForm could just return the JsonResponse object instead of calling send() and exit, but doing this results in the page being rendered in HTML. Is the submit handler therefore totally redundant for FormTestInputForgeryForm?

I'm uploading a new patch that stops using _form_test_submit_values_json() and removes some more legacy code. I've included the send() and exit code in FormTestCheckboxForm but not in FormTestInputForgeryForm, as per #7.

Re-roll of the patch.

@vijaycs85, it is still missing documentation of the classes yet, the new test form classes are not in the Form subdirectory just because other classes where already there also, but no other special reason.

Are these tests even being run? Or how do I run them?

When I run "drush test-run" I don't see any of the tests that I modified that live on module/system/tests:

- core/modules/system/tests/modules/form_test/src/FormTestCheckboxForm.php

but only other tests that live on module/system/src like:

- core/modules/system/src/Tests/Form/CheckboxTest.php

Am I looking on the wrong place?

22: drupal-convert_to_form_builder_in_form_test-2078867-22.patch queued for re-testing.

@jackbravo, Thanks for your help on this issue!

The classes in the patch are not automated tests themselves. They are "fake" content implementing core functionality, and they are used by test classes to test that functionality. You can figure out where the test forms are used by looking in the routing file in the test module (and the patch) and then seeing what tests use those routes. Maybe a starting point:

[tesla-3:drupal | Wed 21:40:07] $ grep -r "form_test/" * 
core/modules/system/src/Tests/Form/ElementsLabelsTest.php:    $this->drupalGet('form_test/form-labels');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/multiple-true');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/multiple-false');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/colspan');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/empty-text');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalPostForm('form_test/tableselect/multiple-true', $edit, 'Submit');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalPostForm('form_test/tableselect/multiple-true', $edit, 'Submit');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalPostForm('form_test/tableselect/multiple-false', $edit, 'Submit');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-true-default');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-true-no-advanced-select');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-false-default');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-false-advanced-select');
core/modules/system/src/Tests/Form/ElementsVerticalTabsTest.php:    $this->drupalGet('form_test/vertical-tabs');
core/modules/system/src/Tests/Form/ElementsVerticalTabsTest.php:    $this->drupalGet('form_test/vertical-tabs');
core/modules/system/src/Tests/Form/ElementsVerticalTabsTest.php:    $this->drupalGet('form_test/vertical-tabs');
core/modules/system/src/Tests/Form/StateValuesCleanAdvancedTest.php:    $this->drupalPostForm('form_test/form-state-values-clean-advanced', $edit, t('Submit'));
core/modules/system/src/Tests/Form/StateValuesCleanTest.php:    $values = Json::decode($this->drupalPostForm('form_test/form-state-values-clean', array(), t('Submit')));
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalGet('form_test/form-storage');
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalGet('form_test/form-storage', array('query' => array('cache' => 1)));
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalPostForm('form_test/form-storage', array('title' => '', 'value' => 'value_is_set'), 'Continue submit');
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalGet('form_test/form-storage', array('query' => array('cache' => 1)));

Hope this helps!

Ok, gotcha. Thanks @xjm.

I think must of these tests can be activated using

drush test-run "Drupal\system\Tests\Form\FormTest"

I'm getting some fails now on my local dev. I'm gonna change status to send the patch to the test bot again just to verify.

Sending last patch to the testbot.

Actually, the command that ran perfectly from drush was:

sudo -u www-data drush -l drupal8.axai.org test-run "Drupal\system\Tests\Form\FormTest"

There could be some cleanup now, like some routes are form_test while others are form-test. I can add this to the patch.

What else is missing?

Ok, I see what's missing. I'll try to migrate other functions to the new module.

Also here is the new patch with the standarized test-form path instead of test_form.

Here's a reroll.

How is this different from #2030165: Convert form_test_* functions to classes?

Okay this is just the underscore-prefixed ones.

This one is done, might as well get it done now before starting in on the other issue.

As I just rerolled, I feel comfortable RTBCing this. Good work everyone!

Committed and pushed to 8.x. Thanks!

I think this still had a lot to go. More test cases need to be migrated. But that can be continued on #2030165: Convert form_test_* functions to classes so I wont reopen this.