Problem/Motivation

Making this a bug in the test helper trait as it is pretty useless like this now as phpunit fails on the first message:

this part:

    if (!$match) {
      debug('Unwanted cache contexts in response: ' . implode(',', array_diff($actual_contexts, $expected_contexts)));
      debug('Missing cache contexts in response: ' . implode(',', array_diff($expected_contexts, $actual_contexts)));
    }

    $this->assertIdentical($actual_contexts, $expected_contexts, $message);

The problem is that phpunit fails on the first error, which includes debug messages, and especially if the problem is *missing* cache contexts, then you only see the first useless message.

Proposed resolution

Just drop that debug stuff, phpunit shows a nice diff in case of assertIdentical() mismatches.

While we're at it, we should however also revert expected/actual so that the diff order makes sense.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

That's true for all those debug() calls in there, including the whole \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait::debugCacheTags() methods so that can possibly just be removed.

msankhala’s picture

@Berdir Does this include debug() call in \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait::assertCacheMaxAge() as well?

Should this complete section be removed?

if (strpos($cache_control_header, 'max-age:' . $max_age) === FALSE) {
  debug('Expected max-age:' . $max_age . '; Response max-age:' . $cache_control_header);
}
While we're at it, we should however also revert expected/actual so that the diff order makes sense.

Can you please elaborate on this?

Berdir’s picture

Yes, that too.

I confused that a bit, what we should use is assertSame() to get the diff, and that expects "assertSame($expected, $actual)", so the expected value must come first.

msankhala’s picture

Status: Active » Needs review
FileSize
2.75 KB

Here is the first version of the patch.

@Berdir we have deprecated core/modules/system/src/Tests/Cache/AssertPageCacheContextsAndTagsTrait.php as well. Should we make this change in that too or leave it because this already deprecated?

Berdir’s picture

We don't want to touch that as that is not using phpunit.

You didn't update the assertIdentical() call to assertSame() yet.

Status: Needs review » Needs work

The last submitted patch, 5: remove-unhelpful-debug-2978261-5.patch, failed testing. View results

msankhala’s picture

Status: Needs work » Needs review
FileSize
5.25 KB
3.37 KB

Here is updated patch

Version: 8.6.x-dev » 8.7.x-dev

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

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

roderik’s picture

Status: Needs review » Needs work

Almost RTBC.

+++ b/core/modules/system/tests/src/Functional/Cache/AssertPageCacheContextsAndTagsTrait.php
@@ -183,9 +161,6 @@ protected function assertCacheContexts(array $expected_contexts, $message = NULL
-    if (strpos($cache_control_header, 'max-age:' . $max_age) === FALSE) {
-      debug('Expected max-age:' . $max_age . '; Response max-age:' . $cache_control_header);
-    }
     $this->assertTrue(strpos($cache_control_header, 'max-age:' . $max_age));

We're losing information here, because assertTrue() won't tell us what is being asserted.

t looks like PHPUnit on failure will output "Failed asserting that $message." - meaning we should pass a descriptive message in the second argument to assertTrue, describing what is expected.
Like e.g. "response contains header 'max-age: $max_age'"

Berdir’s picture

Did run into the same again and created a duplicate that I closed again. I think we can do better on the assertCacheMaxAge() method and use assertContains(). There are actually a few more instances that can be improved like that because assertContains() supports both string-based lookups as well as arrays.

No interdiff because my patch is actually done from scratch as I did that before I remembered about reporting this before. But I think it's identical otherwise.

The one question left to decide is whether this is OK as changes in a test trait. It would break if someone uses this on a simpletest-based test. Do we need to copy and deprecate the old? I always find that pretty ugly.

Looking at http://grep.xnddx.ru/search?text=AssertPageCacheContextsAndTagsTrait and ignoring portfolio_theme, which has a full core folder in its vendor directory, looks like a bunch of modules use it, looks like most that aren't functional tests yet are mine.. (token, file_entity, views_custom_cache_tag).

Status: Needs review » Needs work

The last submitted patch, 12: remove-unhelpful-debug-2978261-12.patch, failed testing. View results

Wim Leers’s picture

I support whichever approach @Berdir feels is best, because he's smart and the one who's most affected :)

Berdir’s picture

> The one question left to decide is whether this is OK as changes in a test trait. It would break if someone uses this on a simpletest-based test. Do we need to copy and deprecate the old? I always find that pretty ugly.

Not sure what I was looking at when writing this, but we have dedicated traits for simpletest & phpunit, so this should be perfectly fine to change, just need to figure out the test problem.

Wim Leers’s picture

What's next here? I already expressed my support in #14 :)

Berdir’s picture

So the problem is the weird assert method \Drupal\Tests\toolbar\Functional\ToolbarCacheContextsTest::assertToolbarCacheContexts() that explicitly checks the return value, which makes no sense, as the separate asserts also already fail (and with phpunit, it never gets there) maybe this was somehow refactored at some point an this was kept.

But.. to make this as uncontroversial as possible, I just restored the @return and return value, without the BC comment.

mondrake’s picture

Added a PHP 7.3.x-dev test run to #17 as it seems this patch may help fixing #2982684-237: Add a composer scaffolding plugin to core.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

So essentially it now removes all debug output that has become useless since the move towards phpunit, which already provides helpful output of the box. Makes sense! 👍

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 199ef1a and pushed to 8.8.x. Thanks!

  • catch committed 199ef1a on 8.8.x
    Issue #2978261 by Berdir, msankhala, Wim Leers, roderik: \Drupal\Tests\...
catch’s picture

Status: Fixed » Closed (fixed)

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