Split from #2835767-61: Media + REST: comprehensive test coverage for Media + MediaType entity types.

@Wim Leers:

Media::setOwnerId() doesn't return the Media entity that's being modified

CommentFileSizeAuthor
#2 2905738-2-interdiff.txt414 bytesAnonymous (not verified)
#2 2905738-2.patch1.11 KBAnonymous (not verified)
#2 2905738-2-test-only.patch722 bytesAnonymous (not verified)

Comments

Anonymous’s picture

Anonymous (not verified) commented

vaplas created an issue. See original summary.

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs review
Issue tags: +blocker
Related issues: +#2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types
StatusFileSize
new 2905738-2-test-only.patch722 bytes
new 2905738-2.patch1.11 KB
new 2905738-2-interdiff.txt414 bytes

I copied the part of #2835767-46: Media + REST: comprehensive test coverage for Media + MediaType entity types. Please add a credit to @Wim Leers, because this is his patch + my test.

The last submitted patch, 2: 2905738-2-test-only.patch, failed testing. View results

The last submitted patch, 2: 2905738-2-test-only.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

IMO this can be committed without test coverage. I'll let a committer decide.

This blocks #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types, which is major, therefore marking major.

  • catch committed 065888c on 8.5.x 
    Issue #2905738 by vaplas, Wim Leers: Media::setOwnerId() doesn't return...

  • catch committed e557682 on 8.4.x 
    Issue #2905738 by vaplas, Wim Leers: Media::setOwnerId() doesn't return...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Okay, @catch committed the test coverage.

catch credited catch.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Argh lost my comment somewhere.

I think it's worth the test coverage because if someone's relying on the return and we regress, it will be fatal, even though the fix is a one-liner.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Ok. Just know that #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types is adding implicit test coverage for this (that's in fact how we noticed this bug).

Status: Fixed » Closed (fixed)

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