Problem/Motivation

I have some doubts on these tests... what's the purpose of looping through

     foreach (['field', 'filter', 'argument', 'sort'] as $handler_type) {

to have 

       $display->expects($this->atLeastOnce())
         ->method('setOption')
        ->with($this->anything(), [

? In the end, it will just pass if the method is only called once, but I suppose we expect at least to be called once per handler.

Proposed resolution

Fix the test so it makes sense.

CommentFileSizeAuthor
#12 3103913-12.patch866 byteslendude
#4 3103913-4.patch4.34 KBlendude
#4 interdiff-3103913-3-4.txt2.47 KBlendude
#3 3103913-3.patch2.72 KBlendude
#2 3103913-2.patch8.14 KBlendude

Comments

mondrake created an issue. See original summary.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3103913-2.patch8.14 KB

I think it should be testing per handler type, so something like this

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 3103913-3.patch2.72 KB

Now without the auto formatting by PHPStorm....

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new interdiff-3103913-3-4.txt2.47 KB
new 3103913-4.patch4.34 KB

And now with ViewExecutableTest::testAddHandler changed to the same logic

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: ViewExecutableTest::testAddHandler and ::testAddHandlerWithEntityField are misusing the mock expectation? » ViewExecutableTest::testAddHandler and ::testAddHandlerWithEntityField are misusing the mock expectation
Status: Needs review » Reviewed & tested by the community

Looks good to me.

  • catch committed 04722fb on 9.0.x 
    Issue #3103913 by Lendude, mondrake: ViewExecutableTest::testAddHandler...

  • catch committed f60d6f7 on 8.9.x 
    Issue #3103913 by Lendude, mondrake: ViewExecutableTest::testAddHandler...

  • catch committed bd90f56 on 8.8.x 
    Issue #3103913 by Lendude, mondrake: ViewExecutableTest::testAddHandler...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 04722fb and pushed to 9.0.x. Thanks! Cherry-picked to 8.9.x and 8.8.x

mondrake’s picture

mondrake
Location 🇮🇹
commented

Huh, I think this fails in D8 when run under PHP 7.0, because the

+++ b/core/modules/views/tests/src/Unit/ViewExecutableTest.php
@@ -359,31 +366,34 @@ public function testAddHandler() {
+    [$view, $display] = $this->setupBaseViewAndDisplay();

syntax is not supported :(, it should remain list($view, $display) = ....

Sorry for missing that.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Fixed » Needs work
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new 3103913-12.patch866 bytes

Ah sorry :( part of the auto formatting by PHPStorm that I didn't catch either

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
commented

I believe this issue is causing failures on PHP 7.0... and you've a hotfix for me already. Thanks!

  • xjm committed 74efa72 on 9.0.x 
    Issue #3103913 followup by Lendude, mondrake, catch, xjm: hotfix for...

  • xjm committed 9f847ae on 8.9.x 
    Issue #3103913 followup by Lendude, mondrake, catch, xjm: hotfix for...

  • xjm committed 4e741be on 8.8.x 
    Issue #3103913 followup by Lendude, mondrake, catch, xjm: hotfix for...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed the hotfix to 9.0.x, 8.9.x, and 8.8.x. (Technically 9.0.x does not need it since PHP 7.0 is not supported, but better to keep the branches in sync as much as possible.)

Thanks everyone!

Status: Fixed » Closed (fixed)

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