ViewExecutableTest::testAddHandler and ::testAddHandlerWithEntityField are misusing the mock expectation [#3103913]

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.

Comment	File	Size	Author
#12	3103913-12.patch	866 bytes	Lendude
#12	8.8.x: PHP 7.1 & MySQL 5.7 28,071 pass PHP 7 & MySQL 5.5 28,071 pass
#4	3103913-4.patch	4.34 KB	Lendude
#4	8.9.x: PHP 7.1 & MySQL 5.7 28,139 pass 8.8.x: PHP 7 & MySQL 5.5 PHPLint Failed
#4	interdiff-3103913-3-4.txt	2.47 KB	Lendude
#3	3103913-3.patch	2.72 KB	Lendude
#3	8.9.x: PHP 7.1 & MySQL 5.7 28,136 pass
#2	3103913-2.patch	8.14 KB	Lendude
#2	8.9.x: PHP 7.1 & MySQL 5.7 CI aborted

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

1 January 2020 at 00:15

mondrake created an issue. See original summary.

Comment #2

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude at iO commented 8 January 2020 at 19:42

Issue summary:	View changes
Status:	Active	» Needs review

File	Size
3103913-2.patch	8.14 KB
8.9.x: PHP 7.1 & MySQL 5.7 CI aborted

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

Comment #3

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude at iO commented 8 January 2020 at 19:45

File	Size
3103913-3.patch	2.72 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,136 pass

Now without the auto formatting by PHPStorm....

Comment #4

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude at iO commented 9 January 2020 at 06:40

File	Size
interdiff-3103913-3-4.txt	2.47 KB
3103913-4.patch	4.34 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,139 pass 8.8.x: PHP 7 & MySQL 5.5 PHPLint Failed

File

Size

interdiff-3103913-3-4.txt

2.47 KB

3103913-4.patch

4.34 KB

8.9.x:
PHP 7.1 & MySQL 5.7 28,139 pass

8.8.x:
PHP 7 & MySQL 5.5 PHPLint Failed

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

Comment #5

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 9 January 2020 at 11:45

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.

Comment #6

10 January 2020 at 14:14

catch committed 04722fb on 9.0.x

Issue #3103913 by Lendude, mondrake: ViewExecutableTest::testAddHandler...

Comment #7

10 January 2020 at 14:16

catch committed f60d6f7 on 8.9.x

Issue #3103913 by Lendude, mondrake: ViewExecutableTest::testAddHandler...

Comment #8

10 January 2020 at 14:16

catch committed bd90f56 on 8.8.x

Issue #3103913 by Lendude, mondrake: ViewExecutableTest::testAddHandler...

Comment #9

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 10 January 2020 at 14:16

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

Comment #10

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 10 January 2020 at 15:04

2 files were hidden/shown/deleted

File	Size
3103913-2.patch	8.14 KB
8.9.x: PHP 7.1 & MySQL 5.7 CI aborted
3103913-3.patch	2.72 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,136 pass

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.

Comment #11

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 10 January 2020 at 15:05

Status:

Fixed

» Needs work

Comment #12

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude at iO commented 10 January 2020 at 16:51

Status:

Needs work

» Needs review

File	Size
3103913-12.patch	866 bytes
8.8.x: PHP 7.1 & MySQL 5.7 28,071 pass PHP 7 & MySQL 5.5 28,071 pass

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

Comment #13

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 10 January 2020 at 18:32

Status:

Needs review

» Reviewed & tested by the community

Comment #14

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 10 January 2020 at 19:56

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

Comment #15

10 January 2020 at 19:58

xjm committed 74efa72 on 9.0.x

Issue #3103913 followup by Lendude, mondrake, catch, xjm: hotfix for...

Comment #16

10 January 2020 at 19:59

xjm committed 9f847ae on 8.9.x

Issue #3103913 followup by Lendude, mondrake, catch, xjm: hotfix for...

Comment #17

10 January 2020 at 19:59

xjm committed 4e741be on 8.8.x

Issue #3103913 followup by Lendude, mondrake, catch, xjm: hotfix for...

Comment #18

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 10 January 2020 at 19:59

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!