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
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff.txt | 2.67 KB | tim.plunkett |
#40 | form-1987716-40.patch | 28.04 KB | tim.plunkett |
Comments
Comment #1
vijaycs85Need 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.
Comment #2
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #3
kgoel CreditAttribution: kgoel commentedGoing to work on this.
Comment #4
kgoel CreditAttribution: kgoel commentedComment #6
kgoel CreditAttribution: kgoel commentedComment #7
kgoel CreditAttribution: kgoel commentedI 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.
Comment #8
xjmThanks 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.
Comment #9
dawehnerThis change feels wrong.
One empty line too much
The title should be moved to the routing yml as well.
Comment #10
kgoel CreditAttribution: kgoel commentedComment #12
kgoel CreditAttribution: kgoel commentedpretty stupid mistake
Comment #14
kgoel CreditAttribution: kgoel commented#12: form_test_double_form-1987716-12.patch queued for re-testing.
Comment #16
kgoel CreditAttribution: kgoel commentedRe-rolled patch after this patch https://drupal.org/node/1987718 went in and also fixed title in routing.yml file.
Comment #18
pfrenssenComment #19
pfrenssenComment #20
dawehnerIf 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
Comment #21
pfrenssenNo 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.
Comment #22
dawehnerAfter 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
Comment #23
pfrenssenComment #24
pfrenssenMoved 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.
Comment #25
pfrenssenRerolled after #2091691: Convert test non-form page callbacks to routes and controllers got in.
Comment #26
vijaycs85Since 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.
Comment #27
pfrenssenThanks for the review! Will work on it now.
Comment #28
pfrenssenJust checked and this is being used in
\Drupal\system\Tests\Form\WrapperTest::testWrapperCallback()
. This is requesting the path which calls wrapperCallback().Comment #29
pfrenssenComment #30
tim.plunkettNormally, this should be:
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.
Comment #32
tim.plunkettOkay, that went in.
Comment #33
tim.plunkett32: form-1987716-32.patch queued for re-testing.
Comment #34
dawehnerIt would be kind of cool to have a separate test for drupal_html_id().
Comment #35
tim.plunkett#34, see #2121713: Move drupal_html_id() and drupal_html_class() to Drupal\Component\Utility
Comment #36
pfrenssenComment #37
tim.plunkettRerolled.
Comment #39
tim.plunkett37: form-1987716-37.patch queued for re-testing.
Comment #40
tim.plunkett@dawehner pointed out that we can wrap batch_get().
Comment #41
dawehnerI really like this tests!
Comment #42
tim.plunkettRetitling.
Comment #43
webchickNice, 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!