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
Comment | File | Size | Author |
---|---|---|---|
#17 | remove-unhelpful-debug-2978261-17-interdiff.txt | 1.21 KB | Berdir |
#17 | remove-unhelpful-debug-2978261-17.patch | 5.42 KB | Berdir |
#12 | remove-unhelpful-debug-2978261-12.patch | 5.88 KB | Berdir |
#8 | interdiff-5-8.txt | 3.37 KB | msankhala |
#8 | remove-unhelpful-debug-2978261-8.patch | 5.25 KB | msankhala |
Comments
Comment #2
BerdirThat'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.
Comment #3
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commented@Berdir Does this include debug() call in \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait::assertCacheMaxAge() as well?
Should this complete section be removed?
Can you please elaborate on this?
Comment #4
BerdirYes, 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.
Comment #5
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere 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?Comment #6
BerdirWe don't want to touch that as that is not using phpunit.
You didn't update the assertIdentical() call to assertSame() yet.
Comment #8
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere is updated patch
Comment #11
roderikAlmost RTBC.
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'"
Comment #12
BerdirDid 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).
Comment #14
Wim LeersI support whichever approach @Berdir feels is best, because he's smart and the one who's most affected :)
Comment #15
Berdir> 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.
Comment #16
Wim LeersWhat's next here? I already expressed my support in #14 :)
Comment #17
BerdirSo 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.
Comment #18
mondrakeAdded 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.
Comment #19
Wim LeersSo 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! 👍Comment #20
catchCommitted 199ef1a and pushed to 8.8.x. Thanks!
Comment #22
catch