Problem/Motivation

Spun off from #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction

It was suggested there to replace ::assertEquals() with ::assertSame() in some of the assertions. Let's do the replacement in the whole test here, which will make the resulting code clearer and more robust at the same time.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#13 2983458-13.patch53.84 KBDinesh18
#9 iterdiff-2983458-6-9.txt1.64 KBpguillard
#9 2983458-9.patch52.34 KBpguillard
#6 interdiff-5-6.txt29.13 KBseanB
#6 2983458-6.patch52.7 KBseanB
#6 interdiff-5-6.txt2.2 KBseanB
#6 2983458-6.patch23.59 KBseanB
#5 interdiff-2983458-2-5.txt8.07 KBpguillard
#5 replace-assertEquals-with-assertSame-2983458-5.patch21.58 KBpguillard
#2 replace-assertEquals-with-assertSame-2983458-2.patch25.55 KBmsankhala
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcoscano created an issue. See original summary.

msankhala’s picture

msankhala CreditAttribution: msankhala as a volunteer and at Material commented
Status: Active » Needs review
FileSize
replace-assertEquals-with-assertSame-2983458-2.patch25.55 KB

Here is the patch which replaces assertEquals with assertSame in MediaSourceTest.

Status: Needs review » Needs work

The last submitted patch, 2: replace-assertEquals-with-assertSame-2983458-2.patch, failed testing. View results

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano as a volunteer commented

I guess when it's a translated text we would want to either keep using assertEquals() or to cast the translated text into a string.

pguillard’s picture

pguillard CreditAttribution: pguillard commented
Status: Needs work » Needs review
FileSize
replace-assertEquals-with-assertSame-2983458-5.patch21.58 KB
interdiff-2983458-2-5.txt8.07 KB

I guess this is more lisible to keep assertEquals()
So we get a mix of the two...

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB commented
Title: Use assertSame() instead of assertEquals() in MediaSourceTest kernel test » Use assertSame() instead of assertEquals() in Media tests
FileSize
2983458-6.patch23.59 KB
interdiff-5-6.txt2.2 KB
2983458-6.patch52.7 KB
interdiff-5-6.txt29.13 KB

Added back some places where we could still use assertSame in Drupal\Tests\media\Kernel\MediaSourceTest. And why not fix it in other tests as well while we're at it.

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB commented

I did not mean to upload 2 versions of the patch with the same name, sorry about that!

Status: Needs review » Needs work

The last submitted patch, 6: 2983458-6.patch, failed testing. View results

pguillard’s picture

pguillard CreditAttribution: pguillard commented
Status: Needs work » Needs review
FileSize
2983458-9.patch52.34 KB
iterdiff-2983458-6-9.txt1.64 KB

Last failing test solved

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Glorious. Thanks!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Needs work

I reviewed the patch and scope with a git diff --staged --color-words.

Committed and pushed to 8.6.x. Thanks!

The patch doesn't cherry-pick cleanly to 8.5.x. Should we create an 8.5.x version, to minimize merge conflicts in backports?

  • xjm committed 00f6554 on 8.6.x 
    Issue #2983458 by seanB, pguillard, msankhala, marcoscano: Use...
Dinesh18’s picture

Dinesh18 CreditAttribution: Dinesh18 as a volunteer commented
Status: Needs work » Needs review
FileSize
2983458-13.patch53.84 KB

Here is the patch for 8.5.x version.

Status: Needs review » Needs work

The last submitted patch, 13: 2983458-13.patch, failed testing. View results

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB commented
Status: Needs work » Fixed

Not sure if we should actually focus our energy on backporting this with 8.6 already in the alpha phase. Let's just close this.

Status: Fixed » Closed (fixed)

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