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

Comments

marcoscano created an issue. See original summary.

msankhala’s picture

msankhala commented
Status: Active » Needs review
StatusFileSize
new 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
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 commented
Status: Needs work » Needs review
StatusFileSize
new replace-assertEquals-with-assertSame-2983458-5.patch21.58 KB
new 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
commented
Title: Use assertSame() instead of assertEquals() in MediaSourceTest kernel test » Use assertSame() instead of assertEquals() in Media tests
StatusFileSize
new 2983458-6.patch23.59 KB
new interdiff-5-6.txt2.2 KB
new 2983458-6.patch52.7 KB
new 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
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 commented
Status: Needs work » Needs review
StatusFileSize
new 2983458-9.patch52.34 KB
new iterdiff-2983458-6-9.txt1.64 KB

Last failing test solved

phenaproxima’s picture

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

Glorious. Thanks!

xjm’s picture

xjm
she/her
Primary language English
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 commented
Status: Needs work » Needs review
StatusFileSize
new 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
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.