Part of #1971384: [META] Convert page callbacks to controllers and sub-set of #2078867: Convert _form_test_* functions to classes

Convert below form callback to new form builder.

form_test_alter_form
form_test_button_class
form_test_checkboxes_radios
form_test_checkboxes_zero
form_test_clicked_button
form_test_color
form_test_email
form_test_empty_select
form_test_form_rebuild_preserve_values_form
form_test_form_state_values_clean_advanced_form
form_test_form_state_values_clean_form
form_test_group_container
form_test_group_details
form_test_group_fieldset
form_test_group_vertical_tabs
form_test_language_select
form_test_limit_validation_errors_form
form_test_load_include_custom
form_test_load_include_menu
form_test_number
form_test_pattern_form
form_test_placeholder_test
form_test_range
form_test_range_invalid
form_test_redirect
form_test_required_attribute
form_test_select
form_test_state_persist
form_test_storage_form
form_test_url #2030165: Convert form_test_url() to a Controller Assigned to: rteijeiro
form_test_validate_form
form_test_validate_required_form
form_test_validate_required_form_no_title

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

juampynr’s picture

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

Title: Convert form_test_url() to a Controller » Covert all form menu items to new form builder in form_test module part 2
vijaycs85’s picture

Assigned: rteijeiro » Unassigned
Status: Closed (won't fix) » Active
mrded’s picture

Assigned: Unassigned » mrded
vijaycs85’s picture

@mrded, are you still working on this?

mrded’s picture

@vijaycs85 yes, it's quite a lot to do

YesCT’s picture

Mrded, quite a lot of these are assigned to you. I would suggest just keeping one and post the work you have so far. It does not have to work or be perfect. Lets get it out where it can be seen and get some feedback on the approach and the details.

mrded’s picture

Assigned: mrded » Unassigned
mrded’s picture

Issue summary: View changes

updating issue with new list of functions

tim.plunkett’s picture

There is a very good chance we don't want to convert most of these, as they should become phpunit tests.
I have an example somewhere, I'll post it when I get back to my laptop.

tim.plunkett’s picture

Um, I thought I posted back here again.

Anyway, see #1987716: Provide a FormTestBase class to allow PHPUnit form tests for an example.

dipen chaudhary’s picture

@tim.plunkett

How does one recognise which ones should be phpunit tests? I was hoping to pick one of these, but now confused. Happy to work on either phpunit conversion or controller conversion, but limited knowledge on deciding which one should be which? Any pointers?

tkuldeep17’s picture

For converting all these menu item into Form object, I am thinking to follow this approach

  • Choose any Testcase in form_test module from Drupal\system\Tests\Form\
  • Convert all the menu items to Form object, then test locally and submit patch.
  • By this way, Testing of Form API will become easy. we have to not run all test cases within Form API.

So I am submitting patch for Drupal\system\Tests\Form\ValidationTest.php test case. Following menu items have been converted into Form object.

form_test_limit_validation_errors_form
form_test_validate_form
form_test_validate_required_form
form_test_pattern_form

tkuldeep17’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
15.21 KB

so I am submitting patch..

vijaycs85’s picture

Fixing a minor issue made on #15

tkuldeep17’s picture

Almost I have all converted menu items into Form object. Tested on my local, these are two test cases which are failing from Drupal\system\Tests\Form\ElementsTableSelectTest.. and not able to find out cause..
So please help me here..

tkuldeep17’s picture

Submitting updated patch..

tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
724 bytes
145.22 KB

Fixing #21.

tkuldeep17’s picture

@vijaycs85

Regarding Name Convention: Currently if form_id was form_test_clicked_button it changed to Form object\Drupal\form_test\Form\FormTestClickedButton
but in some cases form_id was too long like form_test_form_rebuild_preserve_values_form changed to \Drupal\form_test\Form\TestRebuildPreservation.

So at somewhere Name of class generating Form is following form_id, somewhere not..
I think there should be consistency in Form naming convention.

Should I change name of class or not.?

tkuldeep17’s picture

Even after converting these menu Items into Form Object, still there are lot of menus, which has to be converted.

And also Test case classes is using Simpletest not Unittest. I am getting confusion, it will remain as Simpletest or it will be changed to Unittest.

ianthomas_uk’s picture

In #9/#10, @tim.plunkett suggested that we should be replacing these tests with PHPUnit tests, rather than just shuffling round the existing tests, which is what #12-25 have been doing.

But I've seen concern from @sun on IRC that the PHPUnit tests mock too much, so are not really testing what they claim to be. I can certainly understand this view point, e.g. looking at #1987716-40: Provide a FormTestBase class to allow PHPUnit form tests there is a test testUniqueHtmlId(), but the desired behaviour is being mocked elsewhere in the patch (TestFormBuilder::drupalHtmlId()) so isn't that really just testing the mocking code.

I think we need further thought from someone familiar with PHPUnit before we continue with this work.

tim.plunkett’s picture

It depends on what is being tested. We have no full replacement for hook_element_info in PHPUnit, so anything testing the form types themselves would definitely need to be DUTB.

But in the interest of progress, porting these directly would be fine.

xjm’s picture

xjm’s picture

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

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
Issue tags: -Needs reroll +FormInterface
FileSize
94.63 KB

Attempted reroll, but going to work on this also.

tim.plunkett’s picture

So how is this different from #2078867: Convert _form_test_* functions to classes?
I'm worried that this needs to be done from scratch again :(

jackbravo’s picture

It is probably a different subset of the forms. But it would be better to have them on just one issue I guess. Otherwise the first issue that gets in will make the work for the other one harder (a re-roll).

Well, that is my guess. But maybe there are good reasons to separate them.

sun’s picture

Title: Covert all form menu items to new form builder in form_test module part 2 » Convert all form menu items to new form builder in form_test module part 2

Most of this seems to be covered by #2078867: Convert _form_test_* functions to classes already? We probably need an issue title + summary update here.

Nevertheless, fixing the typo in the issue title. :P

tim.plunkett’s picture

Title: Convert all form menu items to new form builder in form_test module part 2 » Convert form_test_* functions to classes

#2078867: Convert _form_test_* functions to classes went in. That just handled the ones prefixed with an underscore.

jackbravo’s picture

More tests needed to be migrated on the other issue. So that work will need to be completed here now I guess.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
60.47 KB

Working on this from scratch. This is just a start, but I want to make sure I'm on the right track while I do more.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
105.3 KB

12 left for tomorrow, this should pass in the meantime.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
164.37 KB

This should be all of them!

Status: Needs review » Needs work

The last submitted patch, 44: form_test-2030165-44.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
164.36 KB
990 bytes

Missed a spot.

scottrigby’s picture

Assigned: tim.plunkett » scottrigby
scottrigby’s picture

Assigned: scottrigby » Unassigned
Status: Needs review » Reviewed & tested by the community

This looks great, and should go in so procedural forms can die the quickest death possible.

BTW on a code standards note: I think for large procedural to class conversions like this, it's silly to hold it up for the 2 spaces some comment lines might go over 80 chars (later if someone wants to open a cleanup task for things like this, it might actually be fun to write a script to do it across many files).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
git ac https://www.drupal.org/files/issues/form_test-2030165-46.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  164k  100  164k    0     0  89572      0  0:00:01  0:00:01 --:--:-- 90197
error: patch failed: core/modules/system/tests/modules/form_test/form_test.module:81
error: core/modules/system/tests/modules/form_test/form_test.module: patch does not apply
Berdir’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
164.43 KB
706 bytes

This conflicted because of a small docblock change, back to RTBC. Manual diff of the patch shows that it's identical except of that change, so back to RTBC.

scottrigby’s picture

EDIT: changed comment. Thanks! #50 works.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, awesome work. Getting this in while it's hot!

Committed and pushed to 8.x. Thanks!

  • webchick committed e008d3a on 8.x
    Issue #2030165 by Berdir, tim.plunkett, vijaycs85, tkuldeep17 |...
scottrigby’s picture

Sweet :)

Status: Fixed » Closed (fixed)

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