Problem/Motivation

Currently assertNoCacheTag exist on WebTestBase but there is no equivalent available for BrowserTestBase, in order to facilitate ease of conversion we should add assertNoCacheTag to assertLegacyTrait

Proposed resolution

Remaining tasks

Create the methods and deprecate them.
Add tests.

User interface changes

API changes

New methods available for conversion.

Data model changes

Blocking these issues

- #2870452: Convert web tests to browser tests for menu_ui module
- #2870439: Convert web tests to browser tests for config module
- #2863644: Convert web tests of block

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Title: Add assertNoCacheTag to assertLegacyTrait » Add assertNoCacheTag/assertCacheContext to assertLegacyTrait
dawehner’s picture

Issue tags: +Novice
legovaer’s picture

The method assertCacheContexts contains a call togetCacheHeaderValues() which on his side contains a call to WebTestBase::drupalGetHeader().

I think we first need to identify how we're going to provide exactly the same functionality within PHPUnit before we can start working on this issue.

prafulla_anurag’s picture

I would like to work on this issue. This would be my first task with drupal, so I might need some help.

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

Priority: Normal » Major
Status: Active » Needs review
FileSize
2.16 KB

This is needed by a lot of tests for a wtb to btb conversion

dawehner’s picture

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -681,6 +681,34 @@ protected function assertCacheTag($expected_cache_tag) {
+    @trigger_error('assertCacheContext() is deprecated and scheduled for removal in Drupal 9.0.0. Use $this->assertSession()->responseHeaderContains() instead.', E_USER_DEPRECATED);
...
+    @trigger_error('assertNoCacheContext() is deprecated and scheduled for removal in Drupal 9.0.0. Use $this->assertSession()->responseHeaderNotContains() instead.', E_USER_DEPRECATED);

Can we link to some change record which explain this kind of conversion? I believe we should add this, given how the trigger_errors should look like.

GoZ’s picture

Change record has been created.

dawehner’s picture

Do you think we should have some trait for those assertions?

GoZ’s picture

Only if every cache asserts are moving to this assertCacheTrait.

1/ We add this asserts to assertLegacyTrait and will move every cache assert in a new assertCacheTrait with another issue.
2/ First we put those asserts to a new assertCacheTrait than we will move other cache assert to assertCacheTrait with another issue.

I have no preference for one or the other.

dawehner’s picture

Issue tags: +Needs tests

It feels like cache tags are so often needed that adding it into the testing api could make sense for itself. Any thoughts?

michielnugter’s picture

I have encountered another test that could use it here:
#2863644: Convert web tests of block.

As there is a valid alternative in Mink I prefer adding it to the AssertLegacyTrait so we can dump them again in Drupal 9.

+1 on the needs tests though, we need to add coverage for these assertions in BrowserTestBaseTest.

michielnugter’s picture

Status: Needs review » Needs work
Torenware’s picture

Issue tags: +Baltimore2017
dawehner’s picture

Does someone mind update the issue summary a bit, at least?

benjifisher’s picture

Issue tags: +Needs reroll

Adding a tag based on #17. Maybe the attention this issue got in #17 is barely enough that I should keep the Baltimore2017 tag.

benjifisher’s picture

andypost’s picture

Maybe better to convert AssertPageCacheContextsAndTagsTrait & deprecate in favour of new one?

Lendude’s picture

Title: Add assertNoCacheTag/assertCacheContext to assertLegacyTrait » Add assertNoCacheTag/assertCacheTag to assertLegacyTrait
Issue summary: View changes
Issue tags: -Novice

Aghhh ok just wrote a different comment 3 times and then when diving in deeper turned completely around on this again and again!

I think that the title should point to assertCacheTag and assertNoCacheTag (@dawehner correct me if I'm wrong please), since these methods exist on WebTestBase, and nowhere else. So not cacheContext. The cacheContext methods are available on AssertPageCacheContextsAndTagsTrait and anything needing those should wait for #2863318: Cache: Convert system functional tests to phpunit to land. So the patch in #10 is not correct (sorry).

Updating the IS with what I think should be done here, but of course update it if I'm reading all of this wrong.

naveenvalecha’s picture

Title: Add assertNoCacheTag/assertCacheTag to assertLegacyTrait » Add assertNoCacheTag, assertCacheContext/assertNoCacheContext to assertLegacyTrait
Issue summary: View changes
Issue tags: +phpunit initiative

We have assertCacheTag in AssertLegacyTrait we need assertNoCacheTag, assertCacheContext/assertNoCacheContext to BTB
Updated IS accordingly.

//Naveen

naveenvalecha’s picture

naveenvalecha’s picture

Lendude’s picture

Status: Needs review » Needs work

@naveenvalecha I think the interdiff in #24 is the patch as it should be. Both assertCacheContext and assertNoCacheContext are (or will be) available on \Drupal\system\Tests\Cache\AssertPageCacheContextsAndTagsTrait, that just needs to be converted. If we add those methods to AssertLegacyTrait we will be in a right mess when you use AssertPageCacheContextsAndTagsTrait once it's converted.

naveenvalecha’s picture

\Drupal\system\Tests\Cache\AssertPageCacheContextsAndTagsTrait is being used by the WTB tests. if we'll convert it for BTB then the WTB tests(which are using this Trait) will fail b/c $this->assertSession() only compatible with the BTB not WTB. So we need to add these methods to the AssertLegacyTrait for now and we rely on the AssertPageCacheContextsAndTagsTrait once all the conversion will be done.

OR

We could add a copy of AssertPageCacheContextsAndTagsTrait into BTB namespace and use it in BTB tests. What do you think?

//Naveen

Lendude’s picture

We could add a copy of AssertPageCacheContextsAndTagsTrait into BTB namespace and use it in BTB tests. What do you think?

That one, and deprecate the old one, but I would consider that work part of #2863318: Cache: Convert system functional tests to phpunit. This issue just needs to worry about assertNoCacheTag since that is not available anywhere for BrowserTestBase and doesn't exist on any traits, only on WebTestBase itself

naveenvalecha’s picture

Title: Add assertNoCacheTag, assertCacheContext/assertNoCacheContext to assertLegacyTrait » Add assertNoCacheTag to assertLegacyTrait
Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.21 KB

updated the IS as per #27

Status: Needs review » Needs work

The last submitted patch, 28: 2795085-28.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
1.16 KB

Here's the correct one.

Lendude’s picture

Status: Needs review » Needs work

Nice. Still needs some test coverage added in \Drupal\Tests\Core\Assert\AssertLegacyTraitTest

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
744 bytes

Let's do that quickly :)

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Nice one.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 2795085-32.patch, failed testing. View results

andypost’s picture

Status: Needs work » Reviewed & tested by the community
naveenvalecha’s picture

Issue tags: -Needs tests +blocker

This is blocker for issues mentioned in IS

larowlan’s picture

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -720,6 +720,22 @@ protected function assertCacheTag($expected_cache_tag) {
+   * @see https://www.drupal.org/node/2864029

We need to update the change record (it is about contexts not tags)

Adding @Lendude to issue credits as he was instrumental in shaping patch

larowlan’s picture

Can someone provide new copy to go in the change record on commit?

Lendude’s picture

Updated the change record to match the actual change.

  • larowlan committed ba0ac0f on 8.4.x
    Issue #2795085 by naveenvalecha, GoZ, dawehner, Lendude: Add...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as ba0ac0f and pushed to 8.4.x. Keep up the great work!

andypost’s picture

Yay! lots of issues unblocked! thanx Lee)

dawehner’s picture

Thank you @larowlan!

naveenvalecha’s picture

Thanks a ton! Lee, it unblocked a couple of issues and will help to gain the momentum to phpunit issues.