Problem/Motivation

Due to a random test failure on GitLab CI I ended up looking at the MediaSourceOEmbedVideoTest.php tests. There are links going to http://www.collegehumor.com which is no longer active and redirects to https://signup.dropout.tv/ dropping whatever resource or path was in ULR path. The tests are thus partially invalid.

Proposed resolution

Use something else as disallowed provider and update all collegehumor paths.
There are also still some http URL's, I guess those can be converted to HTTPS.

Remaining tasks

Create MR

Release notes snippet

None

CommentFileSizeAuthor
#24 3401764-nr-bot.txt85 bytesneeds-review-queue-bot
#23 3401764-23.patch8.47 KBbramdriesen

Issue fork drupal-3401764

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

BramDriesen created an issue. See original summary.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Issue summary: View changes

BramDriesen opened merge request !5405

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Title: Media MediaSourceOEmbedVideoTest is testing dead links » Replace dead URL's in MediaSourceOEmbedVideoTest
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Title: Replace dead URL's in MediaSourceOEmbedVideoTest » Replace CollegeHumor URL's and login in core test cases

Looks like it's used a bit more widespread in the test cases, will make it more general.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Title: Replace CollegeHumor URL's and login in core test cases » Replace CollegeHumor URL's and logic in core test cases
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

Pexels won't work as it only supports embedding images. Also tried twitch but they dropped oembed support after API V5.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Active » Needs review

There we are :) tests are passing again.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Applied the MR and searched for collegehumor + college and all references have been replaced.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

The MR is definitely better than what's in HEAD. However, there's still an element of "testing the internet" in there. These are media tests for remote media so I can see where it comes from; however, I wonder if mocked/fixture resources would be preferable. A media subsystem maintainer might be able to give feedback on that point, so tagging and pinging.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

Valid point @xjm, but I think that's already being worked on in #2964636: Improve oEmbed tests

Meanwhile, OEmbedTestTrait will gain a new method or two, intended for use by test authors, allowing them to easily link an asset URL (such as https://www.youtube.com/watch?v=abcxyz), to a locally hosted -- yet accessible over HTTP -- fixture file, like the ones in core/modules/media/tests/fixtures/oembed/*.html. Once linked, the oEmbed system will fetch data from the local filesystem, thus ensuring that properly written oEmbed tests do not ever call out to the Internet.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Related issues: +#2964636: Improve oEmbed tests
xjm’s picture

xjm
she/her
Primary language English
commented

Is this issue a duplicate, then, or just intended as an intermediate fix to reduce random fails?

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

Not really a duplicate, the issue I referenced is not fixing the fact CollegeHumor is no longer a thing. And that other one is fixing more in dept things. This is indeed an attempt to hopefully fix the random failures of this test, and to replace the site that no longer exists with something that does.

The referenced issue will need a rebase if this gets in, happy to do that as well :)

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Parent issue: » #2829040: [meta] Known intermittent, random, and environment-specific test failures
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Think the main point for me is, this could cause random failures as the site doesn't exist anymore, so if this helps that then I think it's an incremental improvement at least.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Critical

OK, that works for me.

Random fails are critical, so bumping priority.

Thanks!

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

Thanks @xjm and @smustgrave! I'll convert that other issue to a MR once this got in and rebase it to include those changes and try to continue that issue from there.

But I agree, mocking like the other issue is trying to achieve is the "end game" :)

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Replace CollegeHumor URL's and logic in core test cases » Replace CollegeHumor URLs and logic in core test cases

  • xjm committed 8087bc58 on 11.x 
    Issue #3401764 by BramDriesen, xjm, smustgrave: Replace CollegeHumor...

  • xjm committed dd3cad2d on 10.2.x 
    Issue #3401764 by BramDriesen, xjm, smustgrave: Replace CollegeHumor...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs subsystem maintainer review

Based on the above and the presence of the followup, this issue is okay without subsystem maintainer review.

Reviewed closely, and also checked the following to make sure the scope is complete:

[ayrton:maintainer | Thu 12:12:53] $ grep -ri "collegehumor" *
[ayrton:maintainer | Thu 12:14:51] $ find . -name "*collegehumor*" -print

Committed to 11.x and 10.2.x. I also tried to backport it to 10.1.x as a test fix, but it did not cherry-pick cleanly (probably on the dictionary). Setting PTBP for a backport version. Thanks!

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 3401764-23.patch8.47 KB

Adding a patch as it didn't let me create a new issue fork for 10.1.x.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3401764-nr-bot.txt85 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review

Bot seems confused. I'll close the MR and see if that helps.

xjm closed merge request !5405

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +no-needs-review-bot

Reroll for 10.1 seems good.

  • xjm committed 870265ac on 10.1.x 
    Issue #3401764 by BramDriesen, xjm, smustgrave: Replace CollegeHumor...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed the backport to 10.1.x. Thanks!

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

My pleasure 😉

Status: Fixed » Closed (fixed)

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