Provide a TourTestBase class for use by core and contrib modules

Hi Clemens,

I have read through the patch and I think we might be able to refactor it to (instead of loading the yml file) check what tips are on the page and ensure it's elements are available.

Ill write a patch to demonstrate this more clearly but the basic idea is:

1) I go to the page.
2) I get the tips from the markup.
3) I then compare and ensure those tips have the ID on the page.

Ill continue reading through and will produce a patch.

Let's use a test view here instead of coupling this to the frontpage view. We can use the test views that already exist.

Also, the class name should be TourTestBase, not TourBaseTest.

Here is my first patch that takes the approach discussed in #15 and #16. We should also add a test for Views UI here as a proof of concept.

Here is the latest patch. We NEED the cssSelect() for tip evaluation (provided in #2030369).

This patch includes a working example of the base class in action.

I have also provided a fail for the instance when no tips are found on the page in which the check is instantiated. This will help with path matching fails eg. what we have with views right now.

I have attached a FAIL and PASS patch.

#1959660: Replace xpath() with WebTestBase::cssSelect() by leveraging Symfony CssSelector is where we're adding a cssSelect method, albeit just using the existing one provided by PHP Unit in core atm.
The Symfony one was originally proposed and rejected in that issue. #2030369: Add Symfony CssSelector to core is a duplicate in my opinion.
Looks like the tests didn't do what was expected :(

Thanks larowlan!!!!

Here we go. I have also refactored the existing Views UI tour test to use our new base class.

FAIL - I am breaking the Views UI tour intentionally to show the base class failing (cannot find tips).
PASS - Checks the page for tour tips and corresponding elements.

This looks great

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -0,0 +1,63 @@
+   *   A list of tips which provide either a "data-id" or "data-class" for testing.
...
+    // Using xpath, get the rendered tips and there data-id and data-class attributes.

Needs to wrap at 80 chars

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -0,0 +1,63 @@
+   *   Advanced xample:

Example

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -0,0 +1,63 @@
+   *     $tips = array();
+   *     $tips[] = array('data-id' => 'foo');
+   *     $tips[] = array('data-id' => 'bar');
+   *     $tips[] = array('data-class' => 'baz');
+   *     $this->assertTourTips($tips);

Should this be wrapped in @code/@endcode?

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -0,0 +1,63 @@
+      $rendered_tips = $this->xpath('//ol[@id = "tour"]//li');

Can we get a comment along the lines 'Tips are rendered as <li> elements inside <ol id="tour">' just to make it clear?

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -0,0 +1,63 @@
+      $this->assertTrue(FALSE, 'Could not find tour tips on the current page.');

$this->fail() is available

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -0,0 +1,63 @@
+          $elements = \PHPUnit_Util_XML::cssSelect('.' . $tip['data-class'], TRUE, $this->content, TRUE);

::cssSelect ftw

Do you think it would be possible to have another test class that extends this that looks for $this->tips and $this->paths?
Then each module could just extend that class and have the test write it self eg:

abstract class TourTestSimple extends TourTestBase {
  
  public function testTips() {
    foreach ($this->paths as $delta => $path) {
      if (!empty($this->tips[$delta])) {
        $this->drupalGet($path);
        $this->assertTips($this->tips['delta']);
      }
    }
  }
}

class FooTourTest extends TourTestSimple {
    public function getInfo(){}
    public static modules = array('bar');
    protected $paths = array('/foo', '/foo/bar');
    protected $tips = array(
      array(
        array('data-id' => 'foo'),
        array('data-class' => 'bar'),
      ),
      array(
        array('data-id' => 'foo-bar'),
        array('data-class' => 'bar'),
        array('data-class' => 'baz'),
      ),
    );
}

Thoughts?

That's a great idea!

Here is the patch :)

I have tested this against forum issue #1926296 with the following test class: https://gist.github.com/anonymous/a38ba00c172ac0a137d6 (it's in gist form because it's not appropriate for this issue and I will be committing it to the forum issue instead).

Awesome, a few nitpicks

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -19,21 +19,25 @@ class TourTestBase extends WebTestBase {
+   *   A list of tips which provide either a "data-id" or "data-class" ¶

Ouch whitespace

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -19,21 +19,25 @@ class TourTestBase extends WebTestBase {
+    // Get the rendered tips and there data-id and data-class attributes.

their :(

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -43,7 +47,7 @@ public function assertTourTips($tips = array()) {
+      $this->fail(t('Could not find tour tips on the current page.'));

Don't use t in assert messages

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestSimple.phpundefined
@@ -0,0 +1,68 @@
+   * An admin user with administrative permissions for views.

the tour?

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestSimple.phpundefined
@@ -0,0 +1,68 @@
+   * Create an admin user to view tour tips.

I think docblocks for ::setUp are frowned upon, so we should move this comment inside the function (inline comment)

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestSimple.phpundefined
@@ -0,0 +1,68 @@
\ No newline at end of file

Missing newline at end of file

edit: removed duplicate from dreditor

Here are the fixes from #29.

More nitpicks (sorry)

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -19,7 +19,7 @@ class TourTestBase extends WebTestBase {
-   *   A list of tips which provide either a "data-id" or "data-class" 

Needs a .

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestSimple.phpundefined
@@ -65,4 +63,4 @@ public function testTips() {
\ No newline at end of file

Missing newline

Here we go. However, I must have not included the "new line" fix in my previous patch (I have it as a commit locally). It's in the full patch.

I think this is ready (unless bot says no)

1) The advanced example is for the testing of tours that use wildcards.
2) We are testing to make sure that an element does not disappear from the page.
3) These tips are added to the "tour_test" module. Which is enabled for testing tour. We are using the "container" to provide corresponding elements on the page.

Here is the patch as per #36.

The Tour API documentation page should be updated *after* this is committed to core. Otherwise people will start to write tests and get confused because it is not committed.

As per #28 this gist could be used for that documentation: https://gist.github.com/anonymous/a38ba00c172ac0a137d6

unless bot disagrees, this is back to RTBC

So we're adding an abstract class we no use case in core - this is dangerous - we have no idea if it works. There should be at least one use case - just to test it.

Ok, I have removed the TourTestSimple and will implement in a different ticket which has Tour Tips to test.

I also found some additional checks for the assertTips for id's and classes (see interdiff).

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -54,11 +54,11 @@ public function assertTourTips($tips = array()) {
+          $this->assertTrue(count($elements) === 1 && $elements != FALSE, format_string('Found corresponding page element for tour tip with id #%data-id', array('%data-id' => $tip['data-id'])));

!empty($elements) && count($elements) == 1?
I had to read that a few times to make sense of it

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -54,11 +54,11 @@ public function assertTourTips($tips = array()) {
+          $this->assertTrue($elements != FALSE, format_string('Found corresponding page element for tour tip with class .%data-class', array('%data-class' => $tip['data-class'])));

This would be better as assertFalse

Thanks larowlan!

Unless bot disagrees

#42: 2028535-42-tourtestbase.patch queued for re-testing.

#42: 2028535-42-tourtestbase.patch queued for re-testing.

Unrelated fails.

- HTTP response expected 204, actual 400
- Value 'aQ{aB`_-' is equal to value '"9Hs>XGN

Rerunning.

yeah see #2056713: Random test failure in Drupal\rest\Tests\NodeTest

#42: 2028535-42-tourtestbase.patch queued for re-testing.

Back to Rtbc

The issue summary seems to be out of date as the code example uses methods which do not exist and are not added by this patch.

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBase.phpundefined
@@ -0,0 +1,67 @@
+   * // Advanced example. The following would be used for multipage or
+   * // targetting a specific subset of tips.
+   * $tips = array();
+   * $tips[] = array('data-id' => 'foo');
+   * $tips[] = array('data-id' => 'bar');
+   * $tips[] = array('data-class' => 'baz');
+   * $this->assertTourTips($tips);
+   * @endcode
+   */
+  public function assertTourTips($tips = array()) {

What's weird is that we've got usage of the advanced usage of this function. Therefore we have no idea if it works :) We need some usage in a test to confirm this works.

Updated the example to patch #3

Update summary to resemble new TourTestBase

I have updated the issue summary. Will update the tests first thing tomorrow.

Thanks alexpott! Here is the addition to test the advanced implementation.

Fixes missing ?>

The following adds back in a class that will aid tour creators in writing tests for tip content.

I have also included an example in the description.

I've reviewed this in length several times and I think we have the best of both worlds here.
We have a new assert helper for use in complex and simple cases.
We have a TourTestBasic class that can be easily extended where all we care about is making sure the elements we're attaching tour tips to still exist on the page, ie to protect against markup/path changes breaking existing tours.
The class name was discussed in irc and I think TourTestBasic fits far better than TourTestSimple.
There's a tour sprint at DrupalGov in around 10 days or so, getting this in before then will be a huge win.

Update summary to include basic tour class example

Committed to 8.x. Thanks!

1. +++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTest.php
   @@ -8,12 +8,12 @@
   +use Drupal\tour\Tests\TourTestBase;
   

   This is actually unused.

2. +++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBasic.php
   @@ -0,0 +1,66 @@
   +namespace Drupal\tour\Tests;
   ...
   +use Drupal\tour\Tests\TourTestBase;
   

   We don't need to use something when inside the same namespace.

Update from TourTestSimple to TourTestBasic

59: 2028535-59-tour-test-unused.patch queued for re-testing.

Follow-up looks fine!

Committed 9279766 and pushed to 8.x. Thanks!