Convert web tests to browser tests for contact module

Let's unassign, so someone else could pick it up.
@naveenvalecha of course you can pick it up at any point in time :)

How about the tests that are extending viewsTestBase, should we cover this in chunks OR rely on views module conversion first

Let's do that in their own subissue. They will probably take some time, given the speciality of views.

How about the tests that are extending viewsTestBase, should we cover this in chunks OR rely on views module conversion first ? how are you dealing with those ? cc / @dawehner / @klausi ?

I totally believe that we should have a dedicated issue for those issues.

    $i = 0;
    foreach ($this->xpath('//table/tbody/tr') as $row) {
      if (((string) $row->td[0]->a) == $label) {
        break;
      }
      $i++;
    }

    $this->clickLink(t('Manage fields'), $i);

This code in ContactSidewideTest is now problematic, as clickLink() no longer supports $i.

1. +++ b/core/modules/contact/tests/src/Functional/ContactAuthenticatedUserTest.php
   --- a/core/modules/contact/src/Tests/ContactLanguageTest.php
   +++ b/core/modules/contact/tests/src/Functional/ContactLanguageTest.php
   
   +++ b/core/modules/contact/tests/src/Functional/ContactLanguageTest.php
   +++ b/core/modules/contact/tests/src/Functional/ContactLanguageTest.php
   @@ -1,8 +1,8 @@
   
   @@ -13,7 +13,7 @@
     */
   -class ContactLanguageTest extends WebTestBase {
   +class ContactLanguageTest extends BrowserTestBase {
   

   Note: There are methods which no longer support a $message, like.

   $this->assertResponse(200, 'The page exists');

   Let's drop those unused parameters.

2. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -1,19 +1,24 @@
   +class ContactPersonalTest extends BrowserTestBase {
   +
   +  use AssertMailTrait {
   +    getMails as drupalGetMails;
   +  }
    
   

   Let's rather rename the usages

PS: @naveenvalecha Do you mind including review only patches? They are really helpful to focus.

@naveenvalecha
I think the right strategy is to add the helper function to WebAssert in Drupal itself ...

Partial work. Still fails because it seems that \Behat\Mink\WebAssert::pageTextNotContains() is not correctly extracting the text from the HTML page.

Thank you for this patch!

Please put

[diff]
  renames = copies

into your git config, see https://www.drupal.org/documentation/git/configure

Also note: This patch includes a book module change, even this issue is just about contact module :)

@dawehner, yes I noticed. The patch is not usable. Sorry, I'll get back.

Here's the correct patch. interdiff is OK from #14

1. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -67,7 +70,7 @@ function testSendPersonalContactMessage() {
   -    $mails = $this->drupalGetMails();
   +    $mails = $this->getMails();
   

   we should rename the methods same as SimpletTest does so that we don't need to change this line.

2. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -204,10 +207,10 @@ function testPersonalContactAccess() {
   -    $this->assertNoFieldChecked('edit-contact--2');
   +    $this->assertSession()->checkboxNotChecked('edit-contact--2');
   

   we should add the assertNoFieldChecked() method to the AssertLegacy trait instead and not change this line.

3. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -204,10 +207,10 @@ function testPersonalContactAccess() {
   -    $this->assertFieldChecked('edit-contact--2');
   +    $this->assertSession()->checkboxChecked('edit-contact--2');
   

   same here.

4. +++ b/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
   @@ -17,9 +18,10 @@
   -class ContactSitewideTest extends WebTestBase {
   +class ContactSitewideTest extends BrowserTestBase {
    
      use FieldUiTestTrait;
   +  use AssertMailTrait;
   

   I think for maximum compatibility we should add AssertMailTRait to BrowserTestBase instead to keep parity with SimpleTest.

5. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -424,4 +577,18 @@ protected function buildXPathQuery($xpath, array $args = array()) {
   +    $this->assertTrue(in_array($expected_cache_context, $cache_contexts), "'" . $expected_cache_context . "' is present in the X-Drupal-Cache-Contexts header.");
   

   use assertCotains() instead.

we should rename the methods same as SimpletTest does so that we don't need to change this line.

Well we should have some BC layer in place, but IMHO putting Drupal on there, just conceptually doesn't make sense.

Rerolled and some progress, but this will still fail. Contains:
* #2784263: Move WTB::assertCacheContext() to AssertPageCacheContextsAndTagsTrait
* #2784427: Browser tests: Plain page text contains CSS styles and JS settings
* #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3)

And we also need #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests for this conversion patch, rolled that in.

Now all contact module tests are converted, except for Views tests and Update Path tests.

1. +++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
   @@ -86,4 +94,22 @@ public function testError() {
   +  /**
   +   * Tests that legacy assertions work.
   +   */
   +  public function testAssertions() {
   
   +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -464,6 +479,103 @@ protected function assertNoFieldChecked($id) {
   +  protected function assertFieldByXPath($xpath, $value = NULL, $message = '') {
   ...
   +  protected function assertNoFieldByXPath($xpath, $value = NULL, $message = '') {
   ...
   +  protected function assertFieldsByValue($fields, $value = NULL, $message = '') {
   

   Shouldn't be here. Part of #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests.

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -57,6 +58,9 @@ protected function assertElementNotPresent($css_selector) {
      protected function assertText($text) {
   +    // Cast MarkupInterface to string.
   +    $text = (string) $text;
   
   @@ -64,6 +68,10 @@ protected function assertText($text) {
   +      // Mink/BrowserKit uses PHP's DOMElement to get the value of an HTML tag.
   +      // It will provide the value HTML entity decoded, so we need to do the
   +      // same here for the text to actually match.
   +      $text = html_entity_decode($text);
   
   @@ -82,6 +90,9 @@ protected function assertText($text) {
      protected function assertNoText($text) {
   +    // Cast MarkupInterface to string.
   +    $text = (string) $text;
   
   @@ -89,6 +100,10 @@ protected function assertNoText($text) {
   +      // Mink/BrowserKit uses PHP's DOMElement to get the value of an HTML tag.
   +      // It will provide the value HTML entity decoded, so we need to do the
   +      // same here for the text to actually match.
   +      $text = html_entity_decode($text);
   
   +++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
   @@ -154,6 +155,52 @@ public function testAssertOptionSelectedFail() {
      /**
   +   * @covers ::assertText
   +   */
   +  public function testAssertText() {
   ...
   +  /**
   +   * @covers ::assertNoText
   +   */
   +  public function testAssertNoText() {
   

   These changes were deferred to #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3).

3. +++ b/core/tests/Drupal/Tests/DocumentElement.php
   @@ -0,0 +1,46 @@
   +/**
   + * Overrides the Mink document to better handle page text.
   + */
   +class DocumentElement extends MinkDocumentElement {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getText() {
   

   So, the parent getText() doesn't remove style and script? Also here the output will contain encoded entities while parent::getText() return decoded entities (as a human would read them). See #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3). I think we should get first parent::getText() and apply changes against that. I'm not sure about the solution and, as we did in every conversion, this should be split in other issue.

Postponing on #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests and #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3).

Re-roll. I'm not 100% certain of the file locations - the previous patch was noticeably different to 8.4.x.

Looks like you forgot to move the files to the tests directory?

As I said, I'm uncertain of the correct file locations, can you give me a hand @klausi? Which files should be where in this case? That should help me for future issues too. Thanks

they should be in tests/src/Functional of the module. See for example https://www.drupal.org/commitlog/commit/2/4c64f7b840ed4cbd1d075b3f409225...

How about this then?

1. +++ b/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
   @@ -386,14 +388,14 @@ function testAutoReply() {
   diff --git a/core/modules/contact/src/Tests/ContactStorageTest.php b/core/modules/contact/tests/src/Functional/ContactStorageTest.php
   
   diff --git a/core/modules/contact/src/Tests/ContactStorageTest.php b/core/modules/contact/tests/src/Functional/ContactStorageTest.php
   similarity index 98%
   
   similarity index 98%
   rename from core/modules/contact/src/Tests/ContactStorageTest.php
   
   rename from core/modules/contact/src/Tests/ContactStorageTest.php
   rename to core/modules/contact/tests/src/Functional/ContactStorageTest.php
   

   A rename is not enough, you also need to change the namespace in this file.

2. +++ b/core/modules/contact/tests/src/Functional/ContactStorageTest.php
   @@ -1,6 +1,6 @@
   diff --git a/core/modules/contact/src/Tests/Update/ContactUpdateTest.php b/core/modules/contact/tests/src/Functional/Update/ContactUpdateTest.php
   
   diff --git a/core/modules/contact/src/Tests/Update/ContactUpdateTest.php b/core/modules/contact/tests/src/Functional/Update/ContactUpdateTest.php
   similarity index 97%
   
   similarity index 97%
   rename from core/modules/contact/src/Tests/Update/ContactUpdateTest.php
   
   rename from core/modules/contact/src/Tests/Update/ContactUpdateTest.php
   rename to core/modules/contact/tests/src/Functional/Update/ContactUpdateTest.php
   

   I think the update path tests are special and we might not be able to convert them in this issue.

3. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -36,11 +36,19 @@ public function testGoTo() {
        // Test page contains some text.
        $this->assertSession()->pageTextContains('Test page text.');
   +    // Make sure CSS <style> content is not considered as part of the page text.
   +    $this->assertSession()->pageTextNotContains('@import');
   +    // Make sure JS setting values are not part of the page text.
   +    $this->assertSession()->pageTextNotContains('"baseUrl"');
    
   

   unrelated change, we should not change BrowserTestBaseTest in this issue.

4. +++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
   index 0000000..323bde2
   --- /dev/null
   
   --- /dev/null
   +++ b/core/tests/Drupal/Tests/DocumentElement.php
   

   same here, this is all coming from other patches form other issues and should not be included in this patch.

1. The namespace was changed as part of #35, is namespace Drupal\Tests\contact\Functional; not correct?
2. I have moved Update/ContactUpdateTest.php back to where is was.
3. These changes came from the patch in #29, as mentioned on another ticket, should I not be basing my patches on the existing patches? Or should I just start from scratch and ignore previous patches?
4. Ditto.

• Removed non-relevant code.
• Returned Views/ tests back to Tests/Views/.

I can reproduce the exception for Drupal\Tests\contact\Functional\ContactSitewideTest

$ php ./core/scripts/run-tests.sh --url http://127.0.0.1:8888 --verbose --color --class "Drupal\Tests\contact\Functional\ContactSitewideTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\contact\Functional\ContactSitewideTest

Test run started:
  Friday, February 24, 2017 - 11:25

Test summary
------------

Drupal\Tests\contact\Functional\ContactSitewideTest            0 passes             1 exceptions             
FATAL Drupal\Tests\contact\Functional\ContactSitewideTest: test runner returned a non-zero error code (2).
Drupal\Tests\contact\Functional\ContactSitewideTest            0 passes   1 fails                            

Test run duration: 1 min 21 sec

Detailed test results
---------------------


---- Drupal\Tests\contact\Functional\ContactSitewideTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Other      phpunit-9.xml        0 Drupal\Tests\contact\Functional\Con
    PHPunit Test failed to complete; Error: PHPUnit 4.8.27 by Sebastian
    Bergmann and contributors.
    
    EE
    
    Time: 1.34 minutes, Memory: 7.00Mb
    
    There were 2 errors:
    
    1) Drupal\Tests\contact\Functional\ContactSitewideTest::testSiteWideContact
    Undefined property: Behat\Mink\Element\NodeElement::$td
    
    /workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php:272
    
    2) Drupal\Tests\contact\Functional\ContactSitewideTest::testAutoReply
    PHPUnit_Framework_Exception: Fatal error: Call to undefined method
    Drupal\Tests\contact\Functional\ContactSitewideTest::drupalGetMails() in
    /workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
    on line 409
    

Same exception verified for Drupal\Tests\contact\Functional\ContactStorageTest

$ php ./core/scripts/run-tests.sh --url http://127.0.0.1:8888 --verbose --color --class "Drupal\Tests\contact\Functional\ContactStorageTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\contact\Functional\ContactStorageTest

Test run started:
  Friday, February 24, 2017 - 11:30

Test summary
------------

Drupal\Tests\contact\Functional\ContactStorageTest             0 passes             1 exceptions             
FATAL Drupal\Tests\contact\Functional\ContactStorageTest: test runner returned a non-zero error code (2).
Drupal\Tests\contact\Functional\ContactStorageTest             0 passes   1 fails                            

Test run duration: 1 min 45 sec

Detailed test results
---------------------


---- Drupal\Tests\contact\Functional\ContactStorageTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Other      phpunit-10.xml       0 Drupal\Tests\contact\Functional\Con
    PHPunit Test failed to complete; Error: PHPUnit 4.8.27 by Sebastian
    Bergmann and contributors.
    
    .EE
    
    Time: 1.73 minutes, Memory: 7.25Mb
    
    There were 2 errors:
    
    1) Drupal\Tests\contact\Functional\ContactStorageTest::testSiteWideContact
    Undefined property: Behat\Mink\Element\NodeElement::$td
    
    /workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php:272
    
    2) Drupal\Tests\contact\Functional\ContactStorageTest::testAutoReply
    PHPUnit_Framework_Exception: Fatal error: Call to undefined method
    Drupal\Tests\contact\Functional\ContactStorageTest::drupalGetMails() in
    /workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
    on line 409

• Fixed failures.
• Removed unused use statement.

More clear.

+1 LGTM - #50 verified

Re-rolled.

Setting back to RTBC in expectation.

I forgot to move the files.

Looks good, thanks!

Committed and pushed 9e85f54 to 8.4.x and 6755fc9 to 8.3.x. Thanks!