Form: Convert system functional tests to phpunit

I converted a few of the tests -- namely, the ones that didn't break in some scary way upon conversion to BrowserTestBase. :) Namely:

• ArbitraryRebuildTest
• AlterTest
• StateValuesCleanTest
• ResponseTest
• ProgrammaticTest
• EmailTest
• ElementsVerticalTabsTest
• ConfirmFormTest
• SystemConfigFormTest
• UrlTest
• ValidationTest

+++ b/core/modules/system/tests/src/Functional/Form/ValidationTest.php
@@ -59,11 +59,13 @@ public function testValidate() {
+    $this->assertSession()
+      ->elementExists('css', 'input[name="form_token"]')
+      ->setValue('invalid_token');

Think this is the best conversion for hidden fields on Mink. I looked to see if there are other ways but this seems the best way. With the comments explaining it I think we have a good way forward on hidden fields.

Rest of the patch looks good so far, nice minimal changes.

And the fail:

16:49:33 PHP Fatal error:  Cannot redeclare class Drupal\Tests\system\Functional\UrlTest in /var/www/html/core/modules/system/tests/src/Functional/Form/UrlTest.php on line 0
16:49:33 
16:49:33 Fatal error: Cannot redeclare class Drupal\Tests\system\Functional\UrlTest in /var/www/html/core/modules/system/tests/src/Functional/Form/UrlTest.php on line 0
16:49:34 FATAL Drupal\Tests\system\Functional\Form\UrlTest: test runner returned a non-zero error code (255)

Checked this one but can't see the reason yet, haven't dug deep though..

I've made further progress on converting these tests, so I'm assigning this to myself for now.

More progress!

Some of the existing tests use AJAX methods, or are otherwise super-awkward to convert, so I'm going to convert those later. But as of this, I'd say about 80% of the form tests are done!

Not bothering with an interdiff because there are so many changes that it'd be kinda pointless. :)

Nice progress. Setting it to N/R to see where testbot fails?

Some of the existing tests use AJAX methods or are otherwise super-awkward to convert, so I'm going to convert those later.

We're doing the conversion of Ajax tests in follow-up if they are not too straightforward.

Here's More progress. Converted more tests. Updated IS according the conversion in the patch. Few failing tests will look into these later today.

Failing Tests:
1)
TriggeringElementTest::testAttemptAccessControlBypass is failing while setting the name of the input field name, but at Mink NodeElement we don't have the option to set the name of any field.
$elements[0]['name'] = 'button1';
2)
TriggeringElementTest::testNoButtonInfoInPost is failing as the error handing in WTB drupalPostForm is different from BTB drupalPostForm. In BTB, it throws error if the button does not exists, so I think we should move it to follow-up issue to discuss it and then do the changes accordingly.

Here's the green patch. Moved TriggeringElementTest back to WTB, see #11 for more details.

//Naveen

1. +++ b/core/modules/system/tests/src/Functional/Form/LanguageSelectElementTest.php
   @@ -45,7 +45,7 @@ public function testLanguageSelectElementOptions() {
            'edit-languages-locked' => LanguageInterface::STATE_LOCKED,
   -        'edit-languages-config-and-locked' => LanguageInterface::STATE_CONFIGURABLE | LanguageInterface::STATE_LOCKED
   +        'edit-languages-config-and-locked' => LanguageInterface::STATE_CONFIGURABLE | LanguageInterface::STATE_LOCKED,
   

   Nitpick: this is an unnecessary change

2. +++ b/core/modules/system/tests/src/Functional/Form/ProgrammaticTest.php
   @@ -1,16 +1,16 @@
     */
   -class ProgrammaticTest extends WebTestBase {
   +class ProgrammaticTest extends BrowserTestBase {
    
   

   It feels like this test could actually be a kernel test, should we open up a follow up for that?

3. +++ b/core/modules/system/tests/src/Functional/Form/StateValuesCleanTest.php
   @@ -25,7 +25,8 @@ class StateValuesCleanTest extends WebTestBase {
   -    $values = Json::decode($this->drupalPostForm('form_test/form-state-values-clean', [], t('Submit')));
   +    $this->drupalPostForm('form_test/form-state-values-clean', [], t('Submit'));
   +    $values = Json::decode($this->getSession()->getPage()->getContent());
   

   I'm curious whether for BC reasons it would actually make sense to return the HTML output here, much like drupalGet does right now. Do you have an opinion about that?

4. +++ b/core/modules/system/tests/src/Functional/Form/ValidationTest.php
   @@ -59,11 +59,13 @@ public function testValidate() {
   -    $edit = [
   -      'name' => 'validate',
   -      'form_token' => 'invalid token'
   -    ];
   -    $this->drupalPostForm(NULL, $edit, 'Save');
   +    // $this->assertSession()->fieldExists() does not recognize hidden fields,
   +    // which breaks $this->drupalPostForm() if we try to change the value of a
   +    // hidden field such as form_token.
   +    $this->assertSession()
   +      ->elementExists('css', 'input[name="form_token"]')
   +      ->setValue('invalid_token');
   +    $this->drupalPostForm(NULL, ['name' => 'validate'], 'Save');
   

   Maybe we could explain that behat/mink is meant to test normal user behaviour, instead of what we do here, which is abuser behaviour.

#15.2 just checked class ProgrammaticTest extends KernelTestBase and it passes without any further changes, so lets not do it in a followup but do it here

1. +++ b/core/modules/system/tests/src/Functional/Form/CheckboxTest.php
   @@ -70,18 +76,22 @@ public function testFormCheckbox() {
   +      $name = (string) $checkbox->getAttribute('name');
   

   no need for the string cast

2. +++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
   @@ -717,8 +740,6 @@ public function testDisabledMarkup() {
   -    $checkbox = $this->xpath('//input[@name="checkboxes[two]"]');
   -    $checkbox[0]['value'] = 'FORGERY';
   

   Why can we remove this and still get an 'illegal choice'? That seems strange to me.

3. +++ b/core/modules/system/tests/src/Functional/Form/LanguageSelectElementTest.php
   @@ -105,10 +105,11 @@ protected function _testLanguageSelectElementOptions($id, $options) {
   +      $this->assertEqual((string) $option->getText(), $option_title);
   

   No need to cast to string

4. +++ b/core/modules/system/tests/src/Functional/Form/ProgrammaticTest.php
   @@ -65,7 +65,7 @@ public function testSubmissionWorkflow() {
   +  private function doSubmitForm($values, $valid_input) {
   

   we usually go for protected any special reason not to do so here?

@Lendude:

Why can we remove this and still get an 'illegal choice'? That seems strange to me.

I had no choice but to remove that, because Mink has no setAttribute() method for manipulating the DOM tree generically. The equivalent forgery-making happens in the actual form class now -- I added a #post_render callback function to change the value of the checkbox after the form is rendered, resulting in the illegal choice.

@phenaproxima++
Those two things are so far apart in the patch that I'd forgotten about that change by the time I got to reviewing the relevant test bit :)

Since that test is really about catching DOM manipulations, shouldn't we have a javascript test that does this? Now that test feels more about catching people doing weird stuff in their PHP code then about catching client side forgeries. I don't think we're losing any coverage here, the original test was just simulating too, but might be worth a follow up to do some real client side forging.

Really great work on this btw, this is not a straight forward one.

Some progress:

• #15.1 Done
• #15.2 Done
• #16.1 Done
• #16.3 Done
• #16.4 Switched to protected

Still to address/answer:

• #15.3
• #15.4
• #16.2 (see #17 & #18)

Apologies, forgot to move ProgrammaticTest into core/modules/system/tests/src/Kernel/Form/ in last patch, doing it now.

@Manuel Garcia some workflow stuff got into the patch in #21

Thanks @Lendude - clearly I need to improve my patch juggling skills...
Here is the correct patch, interdiff on #21 is correct.

wrong namespace :-s

Created a follow-up for the remaining tests, #2900291: Form: Convert system functional tests to phpunit Part 2

About the remaining questions:
#15.3 Opened #2900292: For BC purposes \Drupal\Tests\BrowserTestBase::drupalPostForm should return the HTML of the rendered page, this is a fairly trivial change, so don't think we need to postpone on this
#15.4 I think that should just be general knowledge, so I think we should add the user/abuser to https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial or something, adding it in a comment here seems a little low impact
#16.2 follow up for additional coverage #2900294: Add javascript test to supplement FormTest::testInputForgery

I think this is ready

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestInputForgeryForm.php
   @@ -33,11 +33,30 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['#post_render'][] = [static::class, 'postRender'];
   ...
   +   * It's necessary to alter the rendered form here because Mink does not
   +   * support manipulating the DOM tree.
   

   nice!

2. +++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
   @@ -717,8 +740,6 @@ public function testDisabledMarkup() {
        $this->drupalPostForm(NULL, ['checkboxes[one]' => TRUE, 'checkboxes[two]' => TRUE], t('Submit'));
        $this->assertText('An illegal choice has been detected.', 'Input forgery was detected.');
   

   I think we should add a @see back to the postRender ^ so that its clear what is going on.

For #29.2 - the input forgery is important from a security standpoint so I think docs gate is enough to block commit

After that - good to go.

I'd also expect that a lot of these can be converted to kernel tests ala PathElementTest but that's obviously out of scope here and future enhancements.

@larowlan thanks for the feedback.

Updated the existing @see on the test method which was referencing _form_test_input_forgery() which probably hasn't existed for 5+ years, added an explanation and a @see back to the post render.

@vaplas I agree we need that change to assertFieldByName but doing it here is too much creep. We need a follow up to #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions or even a roll back since we broke BC, but lets not do that change in this issue.

#31 should work.

#2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions was reverted, reupping #31, since this should now be green.

Back to RTBC.

Updated the existing @see on the test method which was referencing _form_test_input_forgery() which probably hasn't existed for 5+ years, added an explanation and a @see back to the post render.

Gold!

Committed as cfbe5da and pushed to 8.5.x.

Cherry-picked as 4bcd0ea and pushed to 8.4.x

More down!