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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

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.

ayelet_Cr’s picture

Status: Closed (won't fix) » Active
kgoel’s picture

Assigned: Unassigned » kgoel

Going to work on this.

kgoel’s picture

Status: Active » Needs review
FileSize
3.43 KB

Status: Needs review » Needs work

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

kgoel’s picture

Assigned: kgoel » Unassigned
kgoel’s picture

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.

xjm’s picture

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.

dawehner’s picture

  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.

kgoel’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
3.45 KB

Status: Needs review » Needs work

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

kgoel’s picture

Status: Needs work » Needs review
FileSize
589 bytes
3.17 KB

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.

kgoel’s picture

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.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.21 KB

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.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
4.27 KB
4.32 KB
  • 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.
dawehner’s picture

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

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
663 bytes
4.31 KB

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.

dawehner’s picture

+++ 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

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
FileSize
3.43 KB
4.38 KB

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.

pfrenssen’s picture

vijaycs85’s picture

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

pfrenssen’s picture

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

Thanks for the review! Will work on it now.

pfrenssen’s picture

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().

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
4.1 KB
895 bytes
tim.plunkett’s picture

Title: Convert form_test_double_form() to a new style controller » Convert test usage of form_test_double_form() to PHPUnit
Issue tags: +PHPUnit
FileSize
220.31 KB
7.3 KB
26.88 KB
+++ 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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
27.13 KB

Okay, that went in.

tim.plunkett’s picture

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

dawehner’s picture

+++ /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().

tim.plunkett’s picture

pfrenssen’s picture

Issue tags: +Needs reroll
tim.plunkett’s picture

Issue tags: -Needs reroll
FileSize
26.31 KB

Rerolled.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

Component: system.module » forms system
Priority: Normal » Major
Issue summary: View changes
FileSize
28.04 KB
2.67 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I really like this tests!

tim.plunkett’s picture

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

Retitling.

webchick’s picture

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.