Convert web tests to browser tests for shortcut module

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -744,4 +745,52 @@ protected function getRawContent() {
+   * Checks to see whether a block appears on the page.
+   *
+   * @param \Drupal\block\Entity\Block $block
+   *   The block entity to find on the page.
+   *
+   * @deprecated Scheduled for removal in Drupal 9.0.0.
+   *   Use $this->xpath() instead and assert that the result is not empty.
+   */
+  protected function assertBlockAppears(Block $block) {
+    @trigger_error('AssertLegacyTrait::assertBlockAppears() is scheduled for removal in Drupal 9.0.0. Use $this->xpath() instead and assert that the result is not empty.', E_USER_DEPRECATED);
+    $result = $this->findBlockInstance($block);
+    $this->assertTrue(!empty($result), format_string('Ensure the block @id appears on the page', ['@id' => $block->id()]));
+  }
+
+  /**
+   * Checks to see whether a block does not appears on the page.
+   *
+   * @param \Drupal\block\Entity\Block $block
+   *   The block entity to find on the page.
+   *
+   * @deprecated Scheduled for removal in Drupal 9.0.0.
+   *   Use $this->xpath() instead and assert that the result is empty.
+   */
+  protected function assertNoBlockAppears(Block $block) {
+    @trigger_error('AssertLegacyTrait::assertNoBlockAppears() is scheduled for removal in Drupal 9.0.0. Use $this->xpath() instead and assert that the result is empty.', E_USER_DEPRECATED);
+    $result = $this->findBlockInstance($block);
+    $this->assertFalse(!empty($result), format_string('Ensure the block @id does not appear on the page', ['@id' => $block->id()]));
+  }
+
+  /**
+   * Find a block instance on the page.
+   *
+   * @param \Drupal\block\Entity\Block $block
+   *   The block entity to find on the page.
+   *
+   * @return array
+   *   The result from the xpath query.
+   *
+   * @deprecated Scheduled for removal in Drupal 9.0.0.
+   *   Use $this->xpath('//div[@id = :id]', [':id' => 'block-' . $block->id()])
+   *   instead.
+   */
+  protected function findBlockInstance(Block $block) {
+    @trigger_error('AssertLegacyTrait::findBlockInstance() is scheduled for removal in Drupal 9.0.0. Use $this->xpath(\'//div[@id = :id]\', [\':id\' => \'block-\' . $block->id()]) instead.', E_USER_DEPRECATED);
+    return $this->xpath('//div[@id = :id]', [':id' => 'block-' . $block->id()]);
+  }
+
 }

This for me sounds like something we maybe should put into a trait. Any thoughts about that?

May be i should move it into a new AssertBlockTrait. Are you ok with that or you thought about another place ?

+1 from me, but please consult others. I personally believe the \Drupal\simpletest\BlockCreationTrait should be merged with those assertions, as you kinda need both for it to be userful.

+1 form me too.

Agreed.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -744,4 +745,52 @@ protected function getRawContent() {
+    @trigger_error('AssertLegacyTrait::assertBlockAppears() is scheduled for removal in Drupal 9.0.0. Use $this->xpath() instead and assert that the result is not empty.', E_USER_DEPRECATED);
...
+    @trigger_error('AssertLegacyTrait::assertNoBlockAppears() is scheduled for removal in Drupal 9.0.0. Use $this->xpath() instead and assert that the result is empty.', E_USER_DEPRECATED);
...
+    @trigger_error('AssertLegacyTrait::findBlockInstance() is scheduled for removal in Drupal 9.0.0. Use $this->xpath(\'//div[@id = :id]\', [\':id\' => \'block-\' . $block->id()]) instead.', E_USER_DEPRECATED);

Can we point to some change notice?

These methods are added in this issue #2865336: Add \Drupal\simpletest\WebTestBase::assertBlockAppears to \Drupal\FunctionalTests\AssertLegacyTrait.

Shall we postpone on that one and add the methods there?

Is there any way to proceed this? Shall we convert the test which requires #2865336: Add \Drupal\simpletest\WebTestBase::assertBlockAppears to \Drupal\FunctionalTests\AssertLegacyTrait in the followup? otherwise, we'll have to wait for long on long chains of blocking issues? Thoughts?

//Naveen

@naveenvalecha it's only postponed on one issue, that just needs tests, so I think leaving it postponed on that and lets see if we can move #2865336: Add \Drupal\simpletest\WebTestBase::assertBlockAppears to \Drupal\FunctionalTests\AssertLegacyTrait forward, because I think more conversions are blocked on that

The patch in #2 does not apply any more in 8.5.x and 8.4.x so here is a straight re-roll.

However, things have moved on since the discussions above regarding in which Trait to put methods assertBlockAppears and assertNoBlockAppears. The issue #2863644: Convert web tests of block now adds these methods to a new file modules/block/tests/src/Functional/AssertBlockAppearsTrait.php which can be used by the Shortcut tests. That issue is RTBC so it should be not be too long before it is committed. I will create a new patch with the changes required in shortcutLinksTests to use that Trait.

So, here's a patch which uses AssertBlockAppearsTrait.php instead of adding the methods to tests/Drupal/FunctionalTests/AssertLegacyTrait.php. To show that this will pass, I have created the new Block file here in this patch, however, when #2863644: Convert web tests of block lands we will not need to do this, and that bit can be deleted from the patch.

\Drupal\shortcut\Tests\ShortcutTestBase gets moved in the patch. This should not be moved but copied to the new location.

The WebTestBase version should be @deprecated, a trigger_error() should be set, and we need a Change record for the deprecation. We usually treat base classes as API in these conversions.

I just re-rolled the patch, but yes I guess it should be deprecated. Although there are no remaining tests in the old shortcut/src/Tests folder so would anyone really think to create some new WebTestBase test files after these have been removed?

Anyway, I have created a change record https://www.drupal.org/node/2906736 (based on the one for Block module) and made the change so that the base file remains in place, with a deprecation trigger_error.

No changes to the five files from patch #18. New patch attached (with the same note as before, the new AssertBlockAppearsTrait.php file is only to allow testing on d.o. to pass).

@jonathan1055 thanks for the update, looks good, CR looks good too.

Although there are no remaining tests in the old shortcut/src/Tests folder so would anyone really think to create some new WebTestBase test files after these have been removed?

It's about BC, somebody might be using this in contrib. If somebody has a shortcut extending module and has tests, they might be using the base class to build their tests on, and removing the base class would break BC (I'm not saying this is likely here, but for bigger modules like Node and Views this becomes much more likely and IMO it's best to just have a clear policy on this and just use the same deprecation pattern for all conversions).

NB. As a patch-size-shrinker try using git diff --find-copies when you deal with patches that contain these base class copies, makes the patch a little easier to read

Yes of course, I see now, other contrib modules may be extending Shortcut.

Thanks for the hint about git diff --find-copies. That is really good, so here's a smaller much-more-readable patch using it :-)

blocker is in

Here's the straight reroll of the patch with git 3-way merge after #2863644: Convert web tests of block
Also updated IS

Looks great, test bot happy, we happy.

Thanks all!

Yes, I confirm that the patch in #25 is identical to #22 except it does not add the new file AssertBlockAppearsTrait.php. All looks good.

Committed as a739c21 and pushed to 8.5.x.
Cherry-picked as bd28530 and pushed to 8.4.x.

Published change record

Thanks, everyone.