Convert web tests to browser tests for tour module

1. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -0,0 +1,72 @@
   +  public function assertTourTips($tips = array()) {
   

   I guess there is no question that this function is public?

2. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -0,0 +1,72 @@
   +          $this->assertTrue(!empty($elements) && count($elements) === 1, format_string('Found corresponding page element for tour tip with id #%data-id', array('%data-id' => $tip['data-id'])));
   

   What about using $this->assertCount directly here?

3. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -0,0 +1,72 @@
   +          $this->assertFalse(empty($elements), format_string('Found corresponding page element for tour tip with class .%data-class', array('%data-class' => $tip['data-class'])));
   

   assertNotEmpty could be used here

4. +++ b/core/modules/tour/tests/src/Functional/TourTestBasic.php
   @@ -0,0 +1,70 @@
   diff --git a/core/tests/Drupal/FunctionalTests/PageCacheTagsTestBase.php b/core/tests/Drupal/FunctionalTests/PageCacheTagsTestBase.php
   
   diff --git a/core/tests/Drupal/FunctionalTests/PageCacheTagsTestBase.php b/core/tests/Drupal/FunctionalTests/PageCacheTagsTestBase.php
   new file mode 100644
   
   new file mode 100644
   index 0000000..4e1282f
   
   index 0000000..4e1282f
   --- /dev/null
   
   --- /dev/null
   +++ b/core/tests/Drupal/FunctionalTests/PageCacheTagsTestBase.php
   
   +++ b/core/tests/Drupal/FunctionalTests/PageCacheTagsTestBase.php
   +++ b/core/tests/Drupal/FunctionalTests/PageCacheTagsTestBase.php
   @@ -0,0 +1,55 @@
   

   This moving seems to belong into another issue, I guess?

+++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
@@ -0,0 +1,72 @@
+  public function assertTourTips($tips = []) {

Is there a reason this is a public method? I'm just curious.

I guess there's no reason for a public method.

Test failures are because, since #5, #2773389: BrowserTestBase::getTextContent() wrongly returns the raw content has been committed.

This issue for example feels like one which should be ideally totally movable.

I make some changes to conversion :

• I move all simpletest files to phphunit tests.
• I replace array() by []
• I fix some deprecated assert methods like assertText() or assertLink()

Tour module is not the only one to use PageCacheTagsTestBase, which also is used by EntityCacheTagsTestBase and MenuCacheTagsTest. Both are not converted yet. Converting PageCacheTagsTestBase in this patch is required to make tests success, but some duplicate could be found porting the 2 others tests.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -227,7 +227,7 @@ public function titleEquals($expected_title) {
   public function linkExists($label, $index = 0, $message = '') {
     $message = ($message ? $message : strtr('Link with label %label found.', ['%label' => $label]));
-    $links = $this->session->getPage()->findAll('named', ['link', $label]);
+    $links = $this->session->getPage()->findAll('named_exact', ['link', $label]);
     $this->assert(!empty($links[$index]), $message);
   }
 
@@ -248,7 +248,7 @@ public function linkExists($label, $index = 0, $message = '') {

@@ -248,7 +248,7 @@ public function linkExists($label, $index = 0, $message = '') {
    */
   public function linkNotExists($label, $message = '') {
     $message = ($message ? $message : strtr('Link with label %label found.', ['%label' => $label]));
-    $links = $this->session->getPage()->findAll('named', ['link', $label]);
+    $links = $this->session->getPage()->findAll('named_exact', ['link', $label]);
     $this->assert(empty($links), $message);
   }

I don't understand why named_exact is used instead of named. This change in WebAssert make other tests fail.

I made mistake on #14 and #16 between linkNotExists() and linkExists() for 'named_exact' , but i don't think it's a good thing to change this assert forcing exact name check.

In this patch, i prefer to remove the link test on 'Translation' label which return a false negative founding 'Interface Translation' term. This was previously fix in #11 asserting link with 'named_exact'. Another way to improve linkNotExists should be to add an '$is_exact' boolean param, but this should be done in specific issue.

In patch, i also fix TourTestBase namespace in following tests since TourTestBase move to Drupal\Tests\tour\Functional:

• LocaleTranslateStringTourTest
• ViewsUITourTest
• LanguageTourTest

Ok core/modules/language/src/Tests/LanguageTourTest.php should be moved to src/Tests folder but i think it should be done in another issue.

Patch does not apply for me because of #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits, can you reroll?

It's strange but LanguageTourTest.php is not concerned by #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits. So no reroll is needed for the moment.

Re-roll.

Tests will be moved in the following patch...

Move tests from core/modules/tour/src/Tests to core/modules/tour/tests/src/Functional.

1. I haven't moved core/modules/language/src/Tests/LanguageTourTest.php because it is in the language module and this ticket is only about the tour module - is this wrong?
2. Many (if not all) of the changes, such as you've highlighted here, have come from the previous patch, in this case #18. Should I simply ignore previous patches and just move the files and change the namespaces or continue to base on the most recent patch.
3. The change here is from assertEqual() (deprecated) to assertEquals() so should be needed (although, referring back to 2., it could be argued that this is not within the scope of this issue).

Reverted the unrelated change.

I haven't moved any of the tests that are outside the tour module (e.g. core/modules/language/src/Tests/LanguageTourTest.php) - should I?

I have left the assertEquals() - should that be reverted to assertEqual()?

Removed all unrelated changes, mainly [] back to array() and reverting back to deprecated functions.

I think in #28 Jo Fitzgerald named patch interdiff and reverse.

[] back to array() and reverting back to deprecated functions.

There is still a lot of unrelated changes ...

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -25,7 +26,7 @@ class TourTest extends TourTestBasic {
       *   A list of permissions.
       */
   -  protected $permissions = array('access tour', 'administer languages');
   +  protected $permissions = ['access tour', 'administer languages'];
   
   @@ -33,8 +34,9 @@ class TourTest extends TourTestBasic {
      protected $tips = array(
   -    'tour-test-1' => array(),
   +   'tour-test-1' => array(),  ¶
      );
    
   
   @@ -63,34 +65,34 @@ public function testTourFunctionality() {
   -      ':href' => \Drupal::url('<front>', [], ['absolute' => TRUE]),
   +      ':href' => Url::fromRoute('<front>', [], ['absolute' => TRUE])->toString(),
   
   @@ -98,68 +100,68 @@ public function testTourFunctionality() {
   -    $tour = Tour::create(array(
   +    $tour = Tour::create([
          'id' => 'tour-entity-create-test-en',
          'label' => 'Tour test english',
          'langcode' => 'en',
          'module' => 'system',
   -      'routes' => array(
   -        array('route_name' => 'tour_test.1'),
   -      ),
   -      'tips' => array(
   -        'tour-test-1' => array(
   +      'routes' => [
   +        ['route_name' => 'tour_test.1'],
   +      ],
   +      'tips' => [
   +        'tour-test-1' => [
              'id' => 'tour-code-test-1',
              'plugin' => 'text',
              'label' => 'The rain in spain',
              'body' => 'Falls mostly on the plain.',
              'weight' => '100',
   -          'attributes' => array(
   +          'attributes' => [
                'data-id' => 'tour-code-test-1',
   -          ),
   -        ),
   -        'tour-code-test-2' => array(
   +          ],
   +        ],
   +        'tour-code-test-2' => [
              'id' => 'tour-code-test-2',
              'plugin' => 'image',
              'label' => 'The awesome image',
              'url' => 'http://local/image.png',
              'weight' => 1,
   -          'attributes' => array(
   +          'attributes' => [
                'data-id' => 'tour-code-test-2'
   -          ),
   -        ),
   -      ),
   -    ));
   +          ],
   +        ],
   +      ],
   ...
        $dependencies = $tour->calculateDependencies()->getDependencies();
   -    $this->assertEqual($dependencies['module'], array('system', 'tour_test'));
   +    $this->assertEquals($dependencies['module'], ['system', 'tour_test']);
    
        $this->drupalGet('tour-test-1');
   

   These are unrelated changes.

2. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -1,17 +1,17 @@
   -   * Assert function to determine if tips rendered to the page
   -   * have a corresponding page element.
   +   * Assert function to determine if tips rendered to the page have a
   +   * corresponding page element.
   

   Unrelated

I fixed most of these in #28 (hence the massive interdiff), I don't know why @boaloysius ignored that. In his defence, at least his patch didn't fail!

Here's a few more fixed, but I expect there might still be more.

I've found a few more. Sorry for spamming this issue, but I'm finding this is the easiest way for me to do it.

OK, I think this is it now.

The only thing I'm wondering: Could we make another mass moving issue, as some of them seem to be still possible.

+++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
@@ -48,13 +50,14 @@ public function assertTourTips($tips = array()) {
+      $raw_content = $this->getSession()->getPage()->getContent();
       foreach ($tips as $tip) {
         if (!empty($tip['data-id'])) {
-          $elements = \PHPUnit_Util_XML::cssSelect('#' . $tip['data-id'], TRUE, $this->content, TRUE);
+          $elements = \PHPUnit_Util_XML::cssSelect('#' . $tip['data-id'], TRUE, $raw_content, TRUE);
           $this->assertTrue(!empty($elements) && count($elements) === 1, format_string('Found corresponding page element for tour tip with id #%data-id', array('%data-id' => $tip['data-id'])));
         }
         elseif (!empty($tip['data-class'])) {
-          $elements = \PHPUnit_Util_XML::cssSelect('.' . $tip['data-class'], TRUE, $this->content, TRUE);
+          $elements = \PHPUnit_Util_XML::cssSelect('.' . $tip['data-class'], TRUE, $raw_content, TRUE);
           $this->assertFalse(empty($elements), format_string('Found corresponding page element for tour tip with class .%data-class', array('%data-class' => $tip['data-class'])));

I guess this is the most minimal change we can do.

Re-roll patch

@klausi As I asked back in #26 - as part of this ticket, should we be moving files (e.g. core/modules/language/src/Tests/LanguageTourTest.php, core/modules/views_ui/src/Tests/ViewsUITourTest.php & core/modules/locale/src/Tests/LocaleTranslateStringTourTest.php) that are not in the tour module? Won't that be done as part of #2756059: Convert web tests to browser tests for language module & #2747167: Convert first part of web tests of views_ui (is there a ticket for moving the tests in the locale module?)?

@Jo Fitzgerald: right, let's not touch test files of other modules in this issue. Please remove those changes from the patch.

@klausi, just to figure out the workflow for next issues:
In this case, do we have to generate 2 patchs ? One without other modules fixes, which will make tests fail, but the one we will commit at the end of this issue. The other one with other modules fixes so we can pass tests.

Or may be i misunderstand the comment. Here are two cases, please give me the right one, or another if i'm wrong in both.

1/ We should not make any changes in other modules files.
2/ We should only fix dependencies from current issue fix on other modules files. Like i do in #42 patch, using the new fixed namespace in views_ui and language (and why i don't move them)

Not sure it's exactly what you expected, but i prefer to extend the old TestBase test with the new Browser test class instead of copy code.

@klausi in #52 3.

+++ b/core/modules/tour/tests/src/Functional/TourHelpPageTest.php
@@ -135,7 +135,7 @@ protected function getModuleList() {
   protected function getTourList() {
-    return [['Adding languages', 'Language'], ['Editing languages', 'Translation']];
+    return [['Adding languages', 'Language'], ['Editing languages']];
   }

why do we change this line?

I explain this in #18

In this patch, i prefer to remove the link test on 'Translation' label which return a false negative founding 'Interface Translation' term. This was previously fix in #11 asserting link with 'named_exact'. Another way to improve linkNotExists should be to add an '$is_exact' boolean param, but this should be done in specific issue.

I open an issue for the assert link part #2860175: Add exact option to AssertLegacyTrait::assertLink(), WebAssert::linkExists() and WebAssert::linkNotExists()

As discussed offline with @klausi the ViewsUITourTest.php (core/modules/views_ui/src/Tests/ViewsUITourTest.php) is moved into scope for this issue.

@boaloysius why are you leaving empty comments on lots of tickets? My inbox is getting spammed!

@Jo Fitzgerald last night I was updating the organization I am working for. Sorry.

#2860175: Add exact option to AssertLegacyTrait::assertLink(), WebAssert::linkExists() and WebAssert::linkNotExists() is in

Updated patch

- changed link test to exact
- added deprecation trigger as other deprecated tests do

Convert remaining tests that using deprecated \Drupal\tour\Tests\TourTestBase

This looks clean enough :)

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!