Adapt assertCacheContexts to not rely on the return value of assertIdentical

Looking at AssertPageCacheContextsAndTagsTrait for other similar problems

The use of $this->assertIdentical() needs to be removed from assertCacheTags() also

Less unintended coupling - that is a good idea.

I need to signal a slight change is behaviour

previously if assertCacheContext were called with a NULL message then a default message would be constructed inside assertIdentical.

to see this message post patch, we have to rely on the debug function call to see that information. - but that follows the convention setup in the rest of the trait.

belatedly, I agree with #5.

Conventionally methods of the form assertXYZ() don't return a bool

So the next step is to just to assert and not return a boolean.

This has consequences for code that used to make use of the bool.

ToolbarCacheContextsTest::assertToolbarCacheContexts()

Thankfully the chain ends there and nothing uses the return value from assertToolbarCacheContexts()

I have preserved the message - daisy chaining but since there are two assert in assertToolbarCacheContexts the (human) debugger needs to know which point failed.
so I have added a suffix to the message if it is passed in.

-- Did I go too far?

Regarding toolbar tests.... as you wish.

In the next patch I have inserted a Variable name change:- $return becomes $match.

We are actively using it in the method, and I would rather the name have semantic value, and then also happen to be the variable we return.

In relation to #5

Is there a reason why you don't use $this->assert()? assert() is something else.

It is a matter of coupling... looking inside the original call the assertIdentical() - it made use of asser().... Ahem I was just copying
.
$this->assert() implies extending WebAssert, the errors I created in #6 stem from this.

For example look at this stirng of classes
CommentRssTest -- CommentTestBase -- Drupal\simpletest\WebTestBase

At the moment this version of WebTestBase in the string does not pick up webAssert so no assert() and the test fail with a fatal.

In your little unofficial initiative - will this be resolved by other issues?

I'm sorry my knowledge is not wide enough to answer this question.

For the moment I am going to leave the assert() in and post a patch, and say I will be happy to adjust, accordingly.

Maybe we should postpone until other issues resolve the problem?

That works - I am adding documentation - because the next coder along may be confused initially.

Also I want to be careful to "get the cart before the horse" [ I seem to remember there is a similar idiom in german :) ]

In short we have to make the comparison twice - because under failure conditions we have a call to assert in assertIdentical() and I want the debug to show first.

+++ b/core/modules/system/src/Tests/Cache/AssertPageCacheContextsAndTagsTrait.php
@@ -159,12 +159,16 @@ protected function assertCacheContexts(array $expected_contexts, $message = NULL
+
+    // For compatibility with both BrowserTestBase and WebTestBase always return a boolean.
+    return ($return === NULL) ? TRUE : $return;

comments should wrap at 80 characters. Also the comment is wrong, we don't always return a boolean.

Should be "For compatibility with both BrowserTestBase and WebTestBase only return a boolean for WebTestBase."

Which also means we need to adapt the @return docs of this method, right?

Which sounds overly complicated. I think it is fine to always return a boolean, nobody using BrowserTestBase will care.

Yes, always return a boolean and not do this complicated NULL handling in the current patch.

We can just return the $return variable that is explicitly being prepared in the patch.

I don't follow your logic.

if the output of assertIdentical() in some configurations returns null - and the value is used to create $return

how is this compatible with "yes, always return a boolean" ( #15)

Sorry, I meant just return the $match variable like this.

The BTB version of assertIdentical returns NULL.

That's a pretty clear bug, no?

It's very important that assert*() methods return booleans. It's not done very much in core sadly, but good (TM) tests generally fail gracefully, which assert*() methods make very easy if they return booleans.

The following test code, for example, may fail, but it will never fatal:

if ($this->assertInstanceOf($object, $class)) {
  $this->assertSomething($object->someMethodOnClass());
}

Whereas if you can't/don't rely on the return value of assert*() and just do

$this->assertInstanceOf($object, $class);
$this->assertSomething($object->someMethodOnClass());

your test will still never pass incorrectly, but it may fatal which is annoying from a CI/test runner/test logs perspective.

X-Post with @klausi: I think the last patch is a lot more elegant than previous versions, but should I open a separate ticket for the above?

Ah --- that makes the code simpler.

Klausi++

With the move to phpunit we are going to remove return values of assert*() methods to be consistent with phpunit core which never returns anything on assertions.

Fatal errors are fine, since phpunit as a testrunnner can properly deal with them.

The big difference in phpunit is that assertions throw exceptions when they fail. Which means as soon as one assertion fails the test case is finished and will not be continued.

Ahh, yes had totally forgotten that point about PHPUnit, you're right. So my point above is moot. That has always bugged me a lot about PHPUnit, but whatever...

RTBC++

Since this is a test only change - and also makes no functional change - I think this is RC eligible.

Committed and pushed 52f2a29 to 8.3.x and 7ff6bb5 to 8.2.x. Thanks!