Updated: Comment #40

This was previously a conversion issue for form_test_double_form(), that is converted as an example of the new base class

Problem/Motivation

As a part of #1971384: [META] Convert page callbacks to controllers, this was slated to be a direct conversion to controllers.
However, these are forms that do not need web tests, and shouldn't be converted to controllers.

We already have a unit test for forms, but it is for testing the FormBuilder itself. If we want to add more form unit tests, we need a base class.

Proposed resolution

Move the set up and helper code out of FormBuilderTest into a base class
Convert the tests using form_test_double_form() to unit tests

This is major because it provides a base class that we can use to begin converting form web tests to unit tests.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Comments

Status:Active» Closed (won't fix)

Need to rewrite the whole module to make test sync with current test implementation. For more details, please refer: #1988802: [META] Rewrite test modules in system to provide better unit testing.

Status:Closed (won't fix)» Active

Assigned:Unassigned» kgoel

Going to work on this.

Status:Active» Needs review
StatusFileSize
new3.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,402 pass(es), 9 fail(s), and 9 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, form_test_double_form-1987716-4.patch, failed testing.

Assigned:kgoel» Unassigned

I am not sure what needs to be fixed in the patch and how to handle two instances of form which is under form_test_double_form function.

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.

  1. +++ b/core/modules/system/tests/modules/form_test/form_test.module
    @@ -376,9 +370,6 @@ function form_test_permission() {
         ),
    -    'access autocomplete test' => array(
    -      'title' => t('Access autocomplete test'),
    -    ),
       );

    This change feels wrong.

  2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestDoubleForm.php
    @@ -0,0 +1,40 @@
    +

    One empty line too much

+++ b/core/modules/system/tests/modules/form_test/form_test.module
@@ -288,12 +288,6 @@ function form_test_menu() {
-    'title' => 'Double form test',
+++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml
@@ -67,3 +67,10 @@ form_test.autocomplete_2:
+form_test.route9:
+  pattern: '/form-test/double-form'
+  defaults:
+    _form: '\Drupal\form_test\FormTestDoubleForm'
+  requirements:
+    _access: 'TRUE'

The title should be moved to the routing yml as well.

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
new3.45 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/form_test/form_test.module.
[ View ]

Status:Needs review» Needs work

The last submitted patch, form_test_double_form-1987716-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new589 bytes
new3.17 KB
FAILED: [[SimpleTest]]: [MySQL] 59,189 pass(es), 7 fail(s), and 8 exception(s).
[ View ]

pretty stupid mistake

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, form_test_double_form-1987716-12.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, form_test_double_form-1987716-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.21 KB
FAILED: [[SimpleTest]]: [MySQL] 58,299 pass(es), 7 fail(s), and 8 exception(s).
[ View ]

Re-rolled patch after this patch https://drupal.org/node/1987718 went in and also fixed title in routing.yml file.

Status:Needs review» Needs work

The last submitted patch, form_test_double_form-1987716-16.patch, failed testing.

Assigned:Unassigned» pfrenssen

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.27 KB
new4.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,558 pass(es).
[ View ]
  • Changed the router item so it points to a new page controller that outputs the form twice.
  • Renamed the form to FormTestHtmlId
  • Completed the form, it now extends FormBase, and added the required submitForm() method.

Status:Needs review» Needs work

+++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml
@@ -54,6 +54,14 @@ form_test.route8:
+    _controller: '\Drupal\form_test\Controller\FormTestController::twoIdenticalFormInstances'

If you don't return a response object but a render array please use _content. the fact that _controller also works is kind of legacy support at the moment. Sorry

Status:Needs work» Needs review
StatusFileSize
new663 bytes
new4.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,391 pass(es).
[ View ]

No need to say sorry, if it needs work it needs work :D It's great that you took the time to review the patch and provided helpful feedback. I still have much to learn.

It's OK if I keep the page callback in the same class it now is in, right? If it needs to move to a separate class then please let me know.

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
--- /dev/null
+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestHtmlId.php
+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestHtmlId.php
@@ -0,0 +1,46 @@
+ * Contains \Drupal\form_test\FormTestHtmlId.

After looking at most of the other form examples ... they are living in a 'Form' subnamespace, so for this file it would be Drupal\form_test\Form\FormtestHtmlId

Assigned:Unassigned» pfrenssen

Assigned:pfrenssen» Unassigned
StatusFileSize
new3.43 KB
new4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,715 pass(es).
[ View ]

Moved the form and renamed it to FormTestHtmlIdForm, as it is also common to append Form to forms. I also created a followup issue to move the other forms: #2095229: Put form_test forms in Form.

StatusFileSize
new4.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,933 pass(es).
[ View ]

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
@@ -43,10 +44,16 @@ public function wrapperCallback($form_id) {
+      'form1' => drupal_get_form(new \Drupal\form_test\Form\FormTestHtmlIdForm()),
+      'form2' => drupal_get_form(new \Drupal\form_test\Form\FormTestHtmlIdForm()),

Since we have namespace, I guess it is OK to just:
new FormTestHtmlIdForm()

Another note: seems we are not using form_test_wrapper_callback() anywhere, but we have added a helper method Drupal\form_test\Controller\FormTestController::wrapperCallback() to deprecate it. May need to add a followup.

Assigned:Unassigned» pfrenssen
Issue summary:View changes
Status:Needs review» Needs work

Thanks for the review! Will work on it now.

Another note: seems we are not using form_test_wrapper_callback() anywhere, but we have added a helper method Drupal\form_test\Controller\FormTestController::wrapperCallback() to deprecate it. May need to add a followup.

Just checked and this is being used in \Drupal\system\Tests\Form\WrapperTest::testWrapperCallback(). This is requesting the path which calls wrapperCallback().

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,866 pass(es).
[ View ]
new895 bytes

Title:Convert form_test_double_form() to a new style controllerConvert test usage of form_test_double_form() to PHPUnit
Issue tags:+phpunit
StatusFileSize
new220.31 KB
PASSED: [[SimpleTest]]: [MySQL] 57,943 pass(es).
[ View ]
new7.3 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new26.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-1987716-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
@@ -8,6 +8,7 @@
+use Drupal\form_test\Form\FormTestHtmlIdForm;
@@ -43,10 +44,16 @@ public function wrapperCallback($form_id) {
+    return array(
+      'form1' => drupal_get_form(new FormTestHtmlIdForm()),
+      'form2' => drupal_get_form(new FormTestHtmlIdForm()),
+    );

Normally, this should be:

$form_builder = \Drupal::formBuilder();
return array(
  'form1' => $form_builder->getForm('Drupal\form_test\Form\FormTestHtmlIdForm'),
  'form2' => $form_builder->getForm('Drupal\form_test\Form\FormTestHtmlIdForm'),
);

Except, there is no use converting this when the Simpletest should really become a PHPUnit test.

This patch being the first new form phpunit test, it means we have to add the new FormTestBase here.
Also, this needs #2131851: Form errors must be specific to a form and not a global first as well.

Status:Needs review» Needs work

The last submitted patch, 30: form-1987716-30.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.13 KB
PASSED: [[SimpleTest]]: [MySQL] 59,330 pass(es).
[ View ]

Okay, that went in.

32: form-1987716-32.patch queued for re-testing.

+++ /dev/null
@@ -1,45 +0,0 @@
-class HTMLIdTest extends WebTestBase {

It would be kind of cool to have a separate test for drupal_html_id().

Issue tags:+Needs reroll

Issue tags:-Needs reroll
StatusFileSize
new26.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,576 pass(es).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, 37: form-1987716-37.patch, failed testing.

Status:Needs work» Needs review

37: form-1987716-37.patch queued for re-testing.

Component:system.module» forms system
Priority:Normal» Major
Issue summary:View changes
StatusFileSize
new28.04 KB
PASSED: [[SimpleTest]]: [MySQL] 59,842 pass(es).
[ View ]
new2.67 KB

@dawehner pointed out that we can wrap batch_get().

Status:Needs review» Reviewed & tested by the community

I really like this tests!

Title:Convert test usage of form_test_double_form() to PHPUnitProvide a FormTestBase class to allow PHPUnit form tests
Issue summary:View changes

Retitling.

Status:Reviewed & tested by the community» Fixed

Nice, this looks like a great clean-up.

I had some questions about the patch which Tim was able to answer in IRC. So...

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.