Comments

GoZ created an issue. See original summary.

GoZ’s picture

Status: Active » Needs review
FileSize
8.23 KB
dawehner’s picture

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

GoZ’s picture

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

dawehner’s picture

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.

jibran’s picture

+1 form me too.

GoZ’s picture

In any cases, this issue should not move BlockCreationTrait. We should create a new issue to do this.

So we can:
1/ Add assertions to BlockCreationTrait and then move it in a new issue to avoid conflict with other wtb to btb conversions issues.
2/ Add assertions to AssertBlockTrait and then merge BlockCreationTrait in AssertBlockTrait in a new issue.

any thoughts ?

jibran’s picture

Agreed.

dawehner’s picture

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

dawehner’s picture

Status: Needs review » Needs work
michielnugter’s picture

Status: Needs work » Postponed

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?

michielnugter’s picture

michielnugter’s picture

Issue tags: +phpunit initiative
naveenvalecha’s picture

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

Lendude’s picture

@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

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.

jonathan1055’s picture

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.

jonathan1055’s picture

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.

Lendude’s picture

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

jonathan1055’s picture

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

Lendude’s picture

@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

jonathan1055’s picture

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 :-)

larowlan’s picture

Status: Postponed » Active

blocker is in

dawehner’s picture

Status: Active » Needs review
naveenvalecha’s picture

Issue summary: View changes
FileSize
5.77 KB

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

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, test bot happy, we happy.

Thanks all!

jonathan1055’s picture

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.

  • larowlan committed a739c21 on 8.5.x
    Issue #2864088 by jonathan1055, GoZ, naveenvalecha: Convert web tests to...

  • larowlan committed bd28530 on 8.4.x
    Issue #2864088 by jonathan1055, GoZ, naveenvalecha: Convert web tests to...
larowlan’s picture

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

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

Published change record

jibran’s picture

Thanks, everyone.

Status: Fixed » Closed (fixed)

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