CommentFileSizeAuthor
#64 2767275-tour-webtests-64.patch7.92 KBandypost
#64 interdiff.txt1.9 KBandypost
#63 2767275-tour-webtests-63.patch6.02 KBandypost
#63 interdiff.txt1.69 KBandypost
#53 interdiff-2767275-51-53.txt667 bytesGoZ
#53 convert_web_tests_to-2767275-53.patch6.81 KBGoZ
#51 convert_web_tests_to-2767275-51.patch7.21 KBGoZ
#51 interdiff-2767275-49-51.txt2.82 KBGoZ
#49 interdiff-2767275-42-49.txt6.86 KBGoZ
#49 convert_web_tests_to-2767275-49.patch9.71 KBGoZ
#42 convert_web_tests_to-2767275-42.patch7.86 KBGoZ
#39 2767275-39.patch6.67 KBJo Fitzgerald
#39 interdiff-38-39.txt1.18 KBJo Fitzgerald
#38 2767275-38.patch6.78 KBJo Fitzgerald
#38 interdiff-37-38.txt10.17 KBJo Fitzgerald
#37 2767275-37.patch15.84 KBJo Fitzgerald
#37 interdiff-34-37.txt12.12 KBJo Fitzgerald
#34 interdiff-26-34.txt6.12 KBboaloysius
#34 2767275-34.patch25.42 KBboaloysius
#30 interdiff-26-29.txt4.66 KBboaloysius
#30 2767275-29.patch25.44 KBboaloysius
#28 2767275-28.patch4.66 KBJo Fitzgerald
#28 interdiff-26-28.txt25.44 KBJo Fitzgerald
#26 2767275-26.patch28.42 KBJo Fitzgerald
#26 interdiff-22-26.txt545 bytesJo Fitzgerald
#22 2767275-22.patch28.66 KBJo Fitzgerald
#22 interdiff-21-22.txt1.02 KBJo Fitzgerald
#21 2767275-21.patch28.01 KBJo Fitzgerald
#18 interdiff-2767275-16-18.txt1.99 KBGoZ
#18 convert_web_tests_to-2767275-18.patch32.3 KBGoZ
#16 interdiff-2767275-14-16.txt1.11 KBGoZ
#16 convert_web_tests_to-2767275-16.patch48.04 KBGoZ
#14 convert_web_tests_to-2767275-14.patch31.59 KBGoZ
#14 interdiff-2767275-11-14.txt25.88 KBGoZ
#11 interdiff.txt1.61 KBclaudiu.cristea
#11 2767275-11.patch9.02 KBclaudiu.cristea
#9 interdiff.txt624 bytesclaudiu.cristea
#9 2767275-9.patch8.98 KBclaudiu.cristea
#5 interdiff.txt6.71 KBclaudiu.cristea
#5 2767275-5.patch8.98 KBclaudiu.cristea
#2 browsertest-tour-2767275-2.patch9.47 KBklausi
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
9.47 KB

Patch.

dawehner’s picture

  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?

dawehner’s picture

Status: Needs review » Needs work
claudiu.cristea’s picture

FileSize
8.98 KB
6.71 KB
claudiu.cristea’s picture

Status: Needs work » Needs review

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

+++ 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.

claudiu.cristea’s picture

FileSize
8.98 KB
624 bytes

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

Status: Needs review » Needs work

The last submitted patch, 9: 2767275-9.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
9.02 KB
1.61 KB

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

dawehner’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

GoZ’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 14: convert_web_tests_to-2767275-14.patch, failed testing.

GoZ’s picture

Status: Needs work » Needs review
FileSize
48.04 KB
1.11 KB
+++ 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.

Status: Needs review » Needs work

The last submitted patch, 16: convert_web_tests_to-2767275-16.patch, failed testing.

GoZ’s picture

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
klausi’s picture

Status: Needs review » Needs work
index 7778367..06e449f 100644
--- a/core/modules/language/src/Tests/LanguageTourTest.php

--- a/core/modules/language/src/Tests/LanguageTourTest.php
+++ b/core/modules/language/src/Tests/LanguageTourTest.php

+++ b/core/modules/language/src/Tests/LanguageTourTest.php
@@ -2,7 +2,7 @@

@@ -2,7 +2,7 @@
 
 namespace Drupal\language\Tests;
 
-use Drupal\tour\Tests\TourTestBase;
+use Drupal\Tests\tour\Functional\TourTestBase;

this file should be moved to the tests directory and should not be in src/Tests anymore.

Can you make sure that the src/Tests folder is empty when this patch is applied?

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?

GoZ’s picture

Status: Needs work » Needs review

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.

Jo Fitzgerald’s picture

Re-roll.

Tests will be moved in the following patch...

Jo Fitzgerald’s picture

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

The last submitted patch, 18: convert_web_tests_to-2767275-18.patch, failed testing.

klausi’s picture

Status: Needs review » Needs work
  1. diff --git a/core/modules/language/src/Tests/LanguageTourTest.php b/core/modules/language/src/Tests/LanguageTourTest.php
    index 7778367..06e449f 100644
    --- a/core/modules/language/src/Tests/LanguageTourTest.php
    +++ b/core/modules/language/src/Tests/LanguageTourTest.php
    @@ -2,7 +2,7 @@
     
     namespace Drupal\language\Tests;
     
    -use Drupal\tour\Tests\TourTestBase;
    +use Drupal\Tests\tour\Functional\TourTestBase;
    

    looks like you forgot to move this file? EDIT: ah, that is the same file again that has already been moved, right?

  2. +++ b/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    @@ -18,7 +18,7 @@ class TourCacheTagsTest extends PageCacheTagsTestBase {
    -  public static $modules = array('tour', 'tour_test');
    +  public static $modules = ['tour', 'tour_test'];
    

    unrelated change that has nothing to do with this issue. Please only change stuff that is in scope for this issue.

  3. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
    @@ -57,40 +58,40 @@ public function testTourFunctionality() {
    -    $this->assertEqual(count($elements), 1, 'Found English variant of tip 1.');
    +    $this->assertEquals(count($elements), 1, 'Found English variant of tip 1.');
    

    assertEquals() exists on BrowserTestBase now, so this change should not be needed.

Jo Fitzgerald’s picture

  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).
Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
545 bytes
28.42 KB

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()?

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    @@ -54,7 +54,7 @@ public function testRenderedTour() {
    -    $this->pass('Test modification of tour.', 'Debug');
    +    $this->assertTrue('Test modification of tour.', 'Debug');
    

    We shouldn't change this line. We have a pass() method on our AssertLegacyTrait with BrowserTestBase now, so the old line should be fine.

  2. +++ b/core/modules/tour/tests/src/Functional/TourHelpPageTest.php
    @@ -66,17 +66,17 @@ protected function verifyHelp($tours_ok = TRUE) {
    -    $this->assertText('Module overviews are provided by modules');
    +    $this->assertSession()->pageTextContains('Module overviews are provided by modules');
    

    We have assertText() now on BrowserTestBase, so this change should not be needed.

  3. +++ b/core/modules/tour/tests/src/Functional/TourHelpPageTest.php
    @@ -66,17 +66,17 @@ protected function verifyHelp($tours_ok = TRUE) {
    -      $this->assertLink($name);
    +      $this->assertSession()->linkExists($name);
    

    same here, we have assertLink() now.

Can you go through all changes in the patch and check if they are really necessary? We got a lot of compatibility improvements in the meantime.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
25.44 KB
4.66 KB

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

Status: Needs review » Needs work

The last submitted patch, 28: 2767275-28.patch, failed testing.

boaloysius’s picture

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

boaloysius’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: 2767275-29.patch, failed testing.

boaloysius’s picture

Assigned: Unassigned » boaloysius
boaloysius’s picture

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

boaloysius’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
12.12 KB
15.84 KB

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.

Jo Fitzgerald’s picture

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.

Jo Fitzgerald’s picture

Assigned: boaloysius » Unassigned
FileSize
1.18 KB
6.67 KB

OK, I think this is it now.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 2767275-39.patch, failed testing.

GoZ’s picture

Status: Needs work » Needs review
FileSize
7.86 KB

Re-roll patch

klausi’s picture

Status: Needs review » Needs work
diff --git a/core/modules/language/src/Tests/LanguageTourTest.php b/core/modules/language/src/Tests/LanguageTourTest.php
index b11006e..55e49be 100644
--- a/core/modules/language/src/Tests/LanguageTourTest.php
+++ b/core/modules/language/src/Tests/LanguageTourTest.php
@@ -2,7 +2,7 @@
 
 namespace Drupal\language\Tests;
 
-use Drupal\tour\Tests\TourTestBase;
+use Drupal\Tests\tour\Functional\TourTestBase;

Looks like you forgot to move the file. Make sure that you check if all modified test files are at their correct location before you submit the next patch. Thanks!

Jo Fitzgerald’s picture

@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?)?

klausi’s picture

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

GoZ’s picture

@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.

GoZ’s picture

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)

klausi’s picture

1) is the path forward. We don't modify tests of other modules in this issue.

Note that the old TourTestBase class must NOT be removed. It is a testing base class which might be used by other contrib modules, so we need to make a copy of it for browser tests and just deprecate it. Sorry, forgot to point that out in my previous review.

GoZ’s picture

Status: Needs work » Needs review
FileSize
9.71 KB
6.86 KB

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’s picture

Status: Needs review » Needs work
+++ b/core/modules/tour/src/Tests/TourTestBase.php
@@ -2,72 +2,14 @@
+abstract class TourTestBase extends BrowserTourTestBase {

Thanks, but we cannot change the old test base class, it must remain unmodified. It must stay a Simpletest base class, cannot inherit from BrowserTestBase. We can only deprecate it.

GoZ’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
7.21 KB
klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/tour/src/Tests/TourTestBase.php
    @@ -6,6 +6,10 @@
     /**
      * Base class for testing Tour functionality.
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use
    + *   \Drupal\Tests\tour\Functional\TourTestBase instead.
    + *
    + * @see https://www.drupal.org/node/2767275
      */
    

    Before the @deprecated tag should be an empty line. See AggregatorTestBase for an example. Let's use the exact same wording as there: "@deprecated Scheduled for removal in Drupal 9.0.0.
    * Use ... instead."

    And I don't think we need the @see reference to this issue.

  2. +++ b/core/modules/tour/src/Tests/TourTestBase.php
    @@ -6,6 +6,10 @@
    diff --git a/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php b/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    
    diff --git a/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php b/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    index ff2e9e5..daed6fa 100644
    
    index ff2e9e5..daed6fa 100644
    --- a/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    
    --- a/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    +++ b/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    
    +++ b/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    +++ b/core/modules/tour/tests/src/Functional/TourCacheTagsTest.php
    @@ -3,7 +3,7 @@
    
    @@ -3,7 +3,7 @@
     namespace Drupal\Tests\tour\Functional;
     
     use Drupal\Core\Url;
    -use Drupal\system\Tests\Cache\PageCacheTagsTestBase;
    +use Drupal\Tests\system\Functional\Cache\PageCacheTagsTestBase;
    

    Nice find! This class was moved but we forgot to change the base class, so this change is correct.

  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?

  4. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
    @@ -1,6 +1,6 @@
    diff --git a/core/modules/tour/tests/src/Functional/TourTestBase.php b/core/modules/tour/tests/src/Functional/TourTestBase.php
    
    diff --git a/core/modules/tour/tests/src/Functional/TourTestBase.php b/core/modules/tour/tests/src/Functional/TourTestBase.php
    new file mode 100644
    
    new file mode 100644
    index 0000000..522e8e6
    
    index 0000000..522e8e6
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
    
    +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
    +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
    @@ -0,0 +1,73 @@
    

    This should be detected as copy in git. Can you roll the patch again with --find-copies-harder on git diff?

GoZ’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
667 bytes

@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.

klausi’s picture

But that means we are losing test coverage if we just remove this check.

Instead, we have 3 options:
1) change the assertNoLink() calls in this specific test.
2) change the assertNoLink() implementation on AssertLegacyTrait so that it matches the behavior of Simpletest as good as possible.
3) change WebAssert::linkNotExists() to the named_exact solution from #11.

I think option 1 is a no-go because future Simpletest conversion issues should not run into this exact same problem again. So we should open a new issue to either fix assertNoLink() or WebAssert::linkNotExists().

GoZ’s picture

klausi’s picture

Status: Needs review » Postponed

Thanks, postponing on that one.

michielnugter’s picture

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

boaloysius’s picture

Jo Fitzgerald’s picture

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

boaloysius’s picture

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

michielnugter’s picture

Issue tags: +phpunit initiative
larowlan’s picture

andypost’s picture

Status: Active » Needs review
FileSize
1.69 KB
6.02 KB

Updated patch

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

andypost’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks clean enough :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • catch committed 455672e on 8.5.x
    Issue #2767275 by GoZ, Jo Fitzgerald, claudiu.cristea, boaloysius,...
catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.