Test is marked skipped but shouldn't be anymore [#3093757]

There are a few places in the jsonapi tests where there is a reference to issue #2940339. It states that the testRelated test should be re-enabled when that issue is fixed. This didn't happen.

I tried just removing the skips, but it seems there is still some work to be done to make sure the code in testRelated can handle NULL. Currently the tests fails as follows:

The 'data' member was not as expected.
Failed asserting that null is identical to Array &0 ().
 /home/localcopy/drupal/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:789
 /home/localcopy/drupal/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:715
 /home/localcopy/drupal/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:1377
 /home/localcopy/drupal/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:1296
 /home/localcopy/drupal/core/modules/jsonapi/tests/src/Functional/CommentTest.php:396

I added a patch with the tests enabled inline to make debugging easier.

Comment	File	Size	Author
#35	3093757-35.patch	4.59 KB	bbrala
#35	9.1.x: PHP 7.4 & MySQL 5.7 27,593 pass
#33	3093757-33.patch	4.57 KB	bbrala
#33	9.1.x: PHP 7.4 & MySQL 5.7 27,591 pass
#28	interdiff-3093757-22-23.txt	831 bytes	bbrala
#27	interdiff-3093757-22-33.txt	831 bytes	bbrala
#23	3093757-23.patch	4.57 KB	bbrala
#23	9.1.x: PHP 7.4 & MySQL 5.7 Patch Failed to Apply PHP 7.4 & MySQL 5.7 27,511 pass 8.9.x: PHP 7.4 & MySQL 5.7 28,472 pass, 1 fail PHP 7.2 & MySQL 5.7 28,530 pass PHP 7.4 & MySQL 5.7 28,472 pass, 1 fail
#22	3093757-22.patch	3.95 KB	jofitz
#22	9.1.x: PHP 7.4 & MySQL 5.7 27,353 pass 8.9.x: PHP 7.4 & MySQL 5.7 28,530 pass
#22	interdiff-3093757-11-22.txt	1.44 KB	jofitz
#18	interdiff_17-18.txt	2.5 KB	vsujeetkumar
#18	3093757_18.patch	3.21 KB	vsujeetkumar
#18	9.1.x: PHP 7.4 & MySQL 5.7 27,324 pass, 2 fail
#17	interdiff_11-17.txt	1.93 KB	ridhimaabrol24
#17	3093757-17.patch	2.88 KB	ridhimaabrol24
#17	9.1.x: PHP 7.4 & MySQL 5.7 27,263 pass, 6 fail
#11	3093757-11.patch	2.5 KB	bbrala
#11	8.9.x: PHP 7.2 & MySQL 5.5 27,967 pass, 4 fail 9.1.x: PHP 7.4 & MySQL 5.7 27,271 pass, 4 fail
#6	3093757-4.patch	2.21 KB	bbrala
#6	8.9.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply
#2	3093757-1.patch	2.38 KB	bbrala
#2	8.9.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

11 November 2019 at 20:08

bbrala created an issue. See original summary.

Comment #2

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 11 November 2019 at 20:09

File	Size
3093757-1.patch	2.38 KB
8.9.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 11 November 2019 at 20:59

Status:

Active

» Needs work

+++ b/core/modules/jsonapi/tests/src/Functional/BlockContentTest.php
@@ -200,7 +200,8 @@ protected function getExpectedCacheContexts(array $sparse_fieldset = NULL) {
    */
   public function testRelated() {
-    $this->markTestSkipped('Remove this in https://www.drupal.org/project/jsonapi/issues/2940339');
+    // $this->markTestSkipped('Remove this in https://www.drupal.org/project/jsonapi/issues/2940339');
+    parent::testRelated();
   }

You should just remove these methods instead. Also make sure to set the status to needs review when you upload a patch.

Also your patch sees MenuLinkContentTest as a binary file, maybe due to special characters in there? Probably Wim and his love for emojis causing problems again :p

Comment #4

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 11 November 2019 at 21:14

Yeah they should be removed i know, thought it would apply anyways though. Guess not. :)

i'll update the patch.

Comment #5

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 11 November 2019 at 21:11

Issue summary:

View changes

Comment #6

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 11 November 2019 at 21:13

File	Size
3093757-4.patch	2.21 KB
8.9.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply

Comment #7

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 11 November 2019 at 21:15

Status:

Needs work

» Needs review

Comment #8

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 11 November 2019 at 21:16

Status:

Needs review

» Needs work

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 12 November 2019 at 09:47

Issue tags:

+API-First Initiative, +Needs reroll

The patch does not apply, could you please reroll? :)

Comment #10

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 12 November 2019 at 12:06

Yeah will do when i get home, didn't have the energy to fix it yesterday :)

Comment #11

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 12 November 2019 at 15:33

File	Size
3093757-11.patch	2.5 KB
8.9.x: PHP 7.2 & MySQL 5.5 27,967 pass, 4 fail 9.1.x: PHP 7.4 & MySQL 5.7 27,271 pass, 4 fail

File

Size

3093757-11.patch

2.5 KB

8.9.x:
PHP 7.2 & MySQL 5.5 27,967 pass, 4 fail

9.1.x:
PHP 7.4 & MySQL 5.7 27,271 pass, 4 fail

I've forced git to treat all files as text. This should fix the apply.

Comment #12

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented 12 November 2019 at 16:44

Status:

Needs work

» Needs review

Comment #13

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 12 November 2019 at 20:21

This is not a working patch, why should it be 'needs review'?

Comment #14

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented 13 November 2019 at 11:56

Status:

Needs review

» Needs work

Comment #15

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 20 November 2019 at 12:33

Note that the patch from #3088870-25: Add missing REST and JSON:API test coverage for the workspace entity type has the same failure in the testRelated() method as the ones exposed by #11, so this issue should handle that one as well if it gets committed first.

Comment #16

20 November 2019 at 12:33

Version:

8.9.x-dev

» 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Comment #17

ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commented 19 May 2020 at 07:47

File	Size
3093757-17.patch	2.88 KB
9.1.x: PHP 7.4 & MySQL 5.7 27,263 pass, 6 fail
interdiff_11-17.txt	1.93 KB

Comment #18

vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented 26 May 2020 at 13:04

Status:

Needs work

» Needs review

File	Size
3093757_18.patch	3.21 KB
9.1.x: PHP 7.4 & MySQL 5.7 27,324 pass, 2 fail
interdiff_17-18.txt	2.5 KB

Fixing test, Please review.

Comment #19

26 May 2020 at 14:00

Status:

Needs review

» Needs work

The last submitted patch, 18: 3093757_18.patch, failed testing. View results

Comment #20

Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented 27 May 2020 at 08:21

Assigned:

Unassigned

» Deepak Goyal

Comment #21

Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented 1 June 2020 at 04:17

Assigned:

Deepak Goyal

» Unassigned

Comment #22

jofitz CreditAttribution: jofitz at jofitz commented 3 June 2020 at 23:53

Status:	Needs work	» Needs review
Issue tags:	-Needs reroll

File	Size
interdiff-3093757-11-22.txt	1.44 KB
3093757-22.patch	3.95 KB
9.1.x: PHP 7.4 & MySQL 5.7 27,353 pass 8.9.x: PHP 7.4 & MySQL 5.7 28,530 pass

File

Size

interdiff-3093757-11-22.txt

1.44 KB

3093757-22.patch

3.95 KB

9.1.x:
PHP 7.4 & MySQL 5.7 27,353 pass

8.9.x:
PHP 7.4 & MySQL 5.7 28,530 pass

4 files were hidden/shown/deleted

File	Size
3093757-17.patch	2.88 KB
9.1.x: PHP 7.4 & MySQL 5.7 27,263 pass, 6 fail
interdiff_11-17.txt	1.93 KB
3093757_18.patch	3.21 KB
9.1.x: PHP 7.4 & MySQL 5.7 27,324 pass, 2 fail
interdiff_17-18.txt	2.5 KB

After more hours than I'm willing to admit(!), I believe I have come up with the solution to the test failures from the patch in #11 (I think the patches in #17 and #18 are red-herrings - this is a tough issue to debug).

The interdiff may not appear clear immediately, but basically if $relationship_document['data'] is null the the cardinality passed to getEmptyCollectionResponse() should be 1, while currently FALSE is passed.

I believe this will still pass even once #3088870: Add missing REST and JSON:API test coverage for the workspace entity type has been committed.

Comment #23

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 17 June 2020 at 19:20

File	Size
3093757-23.patch	4.57 KB
9.1.x: PHP 7.4 & MySQL 5.7 Patch Failed to Apply PHP 7.4 & MySQL 5.7 27,511 pass 8.9.x: PHP 7.4 & MySQL 5.7 28,472 pass, 1 fail PHP 7.2 & MySQL 5.7 28,530 pass PHP 7.4 & MySQL 5.7 28,472 pass, 1 fail

File

Size

3093757-23.patch

4.57 KB

9.1.x:
PHP 7.4 & MySQL 5.7 Patch Failed to Apply
PHP 7.4 & MySQL 5.7 27,511 pass

I've looked through you patch and it seems like it does fix it that specific instance.

What i did notice is the argument for the getEmptyCollectionResponse is rather weird and might actually be the source of another test that is might not work as expected. Before your patch it is used twice, both times it is fed a boolean. But the check in the method does: 'data' => $cardinality === 1 ? NULL : [], so when a boolean is passed this should always fail and assume an infinite relation.

If the function signature (or at least the dockblock) does imply using 1 or -1 as the value. I rolled a patch with the other call fixed (line 1863 in ResourceTestBase.php).

Comment #24

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 18 June 2020 at 09:33

The one failure is also on the dev branch, so unrelated to this patch.

Comment #25

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 23 June 2020 at 18:44

Status:

Needs review

» Reviewed & tested by the community

The patch is good, I only changed another call. Gonna flag as RTBC.

Comment #26

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 24 June 2020 at 17:50

Status:

Reviewed & tested by the community

» Needs review

Thanks for working on this. It's always great to be able to remove technical debt like this.

@bbrala, in the future, provide interdiffs for your patches. That allows reviewers to evaluate your changes.

Also, in general, it's only allowable to RTBC one's one patch if the change is trivial and does not affect the functionality (e.g., fixing a couple of small coding standards issues or rewording a comment). I'm setting the issue back to "Needs review" for a peer review -- you can upload the interdiff, and other issue contributors can help decide if it also makes sense. Thanks!

Comment #27

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 24 June 2020 at 18:01

File	Size
interdiff-3093757-22-33.txt	831 bytes

Will do, interdiff wasn't available at the moment i created the patch, will take care to always add those in the future..

I was contemplating if i would flag as reviewed since i kinda reviewed his changes and added a little bit, But i'll be a bit more defensive next time. Cheers.

Comment #28

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 24 June 2020 at 18:02

File	Size
interdiff-3093757-22-23.txt	831 bytes

1 file was hidden/shown/deleted

File	Size
interdiff-3093757-22-33.txt	831 bytes

Comment #29

JordanDukart CreditAttribution: JordanDukart as a volunteer commented 12 August 2020 at 14:30

Status:

Needs review

» Reviewed & tested by the community

Reviewed, looks good to me!

Comment #30

14 August 2020 at 16:40

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 23: 3093757-23.patch, failed testing. View results

Comment #31

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 14 August 2020 at 17:47

Status:

Needs work

» Reviewed & tested by the community

As always, the QuickEdit test is failing. Completely unrelated. Settings it back to RTBC

Comment #32

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 28 August 2020 at 08:31

Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs reroll

This needs a re-roll against 9.1.x

Comment #33

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 28 August 2020 at 09:21

Status:

Needs work

» Reviewed & tested by the community

File	Size
3093757-33.patch	4.57 KB
9.1.x: PHP 7.4 & MySQL 5.7 27,591 pass

Unfortunate :)

I rerolled it for 9.1.x, thanks.

Comment #34

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 28 August 2020 at 11:40

Status:

Reviewed & tested by the community

» Needs work

Oof I think I just made this not apply yet again by committing #3072076: JSON:API returns a CacheableResponseInterface instance for non-cacheable methods; causes unnecessary exceptions.... :/

Comment #35

bbrala

Netherlands

CreditAttribution: bbrala at SWIS commented 28 August 2020 at 12:05

Status:

Needs work

» Reviewed & tested by the community

File	Size
3093757-35.patch	4.59 KB
9.1.x: PHP 7.4 & MySQL 5.7 27,593 pass

Hehe, indeed you did @catch, no problem though. A quick fix is possible.

Comment #36

28 August 2020 at 14:04

catch committed 1e06a3e on 9.1.x

Issue #3093757 by bbrala, vsujeetkumar, jofitz, ridhimaabrol24, Berdir,...

Comment #37

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 28 August 2020 at 14:04

Status:

Reviewed & tested by the community

» Fixed

Committed 1e06a3e and pushed to 9.1.x. Thanks!

Comment #38

11 September 2020 at 14:09

Status:

Fixed

» Closed (fixed)

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

Test is marked skipped but shouldn't be anymore

Comments

Parent issue

Related issues