If Vimeo is down our tests break

Didn't really dive into this too deep, but at a first glance we seem to be missing a call to $this->hijackProviderEndpoints(); defined in Drupal\Tests\media\Traits\OEmbedTestTrait for those tests.

Nice find 😁

I started to review this, but these are some pretty special tests, so I think it'd be better for those who helped create them in the first place to sign off on this.

I applied the patch locally and ran MediaStandardProfileTest with my wi-fi turned off. Unfortunately, I still got the same failure as https://www.drupal.org/pift-ci-job/1309043, so I'm not sure the patch is actually fixing the problem...?

Running MediaStandardProfileTest with the patch in #4 applied against 8.8.x HEAD, and wi-fi turned off (I have no other connection to the Internet) gives me this:

Failed asserting that two strings are identical.
Expected :'Drupal Rap Video - Schipulcon09'
Actual   :'example_2.jpeg'

 core/modules/media/tests/src/FunctionalJavascript/MediaStandardProfileTest.php:406
core/modules/media/tests/src/FunctionalJavascript/MediaStandardProfileTest.php:86

EDIT: I get the same failure with the patch in #8.

I wonder if this is not an opportunity to kill two birds with one stone: this issue, and #2964636: Improve oEmbed tests.

The problem, really, is that the OEmbed tests are bizarrely written and extraordinarily fragile (I bear a lot of responsibility for this). They should be rearchitected, IMHO, so that all test data is contained in the media_test_oembed module, and simply by enabling that module, everything "just works" for oEmbed tests. The goals should be, in my opinion:

1. All Media tests should pass without an Internet connection.
2. The only thing that functional oEmbed tests should need to do in order to work is add media_test_oembed to their $modules array.
3. OEmbedTestTrait should be deprecated.

This would make me much happier as a Media maintainer. Thoughts?

I wrote both of the failing tests and apparently it is very easy to do it wrong. It would be great if we can make this a hell of a lot easier. Not sure how big of an effort that would be? I guess having a fix first would at least make sure we don't risk breaking the test bot again when YouTube or vimeo fail. Which should be our main priority now.

I wrote both of the failing tests and apparently it is very easy to do it wrong. It would be great if we can make this a hell of a lot easier. Not sure how big of an effort that would be? I guess having a fix first would at least make sure we don't risk breaking the test bot again when YouTube or vimeo fail. Which should be our main priority now.

The monumental shittiness of the oEmbed tests is mostly my fault. Sorry about that. :(

I'd like to refactor them in #2964636: Improve oEmbed tests as I described in #10, and I'm probably in the best position to do that work. In the meantime, though, if both you and @alexpott are getting passing tests on your local systems with this patch applied and the Internet disconnected, I figure that should be enough to stop the bleeding and would be okay RTBCing the current patch as a temporary fix until we can improve things in the other issue.

Ran the tests multiple times to be sure (with/without the patch in #8). Failed every time without the patch. Green with the patch applied. I think this is good to go.

+1 RTBC I have one suggestion:

--- a/core/modules/media/tests/modules/media_test_oembed/src/Controller/ResourceController.php
+++ b/core/modules/media/tests/modules/media_test_oembed/src/Controller/ResourceController.php
@@ -17,17 +17,21 @@ class ResourceController {
    *   The request.
    *
    * @return \Symfony\Component\HttpFoundation\Response
-   *   The JSON response.
+   *   The JSON response or 404 when resources don't exist or are not available.

Not to derail this issue, but I should mention that I have #2964636: Improve oEmbed tests passing tests, and it allows all oEmbed tests to pass even when the Internet is shut off. So that issue more or less supersedes this one, and greatly reduces the complexity of oEmbed testing. I defer this decision to the committers, but I'm not sure if it would be preferable to commit this patch as-is and stop the immediate bleeding, then reroll #2964636: Improve oEmbed tests...or if we should close this issue, transfer the credit, and finish the other one.

Just thought I'd throw that out there.

I agree with @phenaproxima, making it not rely on the internet in a more complete fashion feels like a better approach - given the likelihood of another vimeo outage is low, perhaps it would be better to focus on that patch instead?

It looks like #2964636: Improve oEmbed tests will be going through its paces now that there's something to iterate, and the priority is preventing core from testing the Internet. So +1 for landing this now.

Reroll

Interdiff between #17 and #24 is failing. So here's a raw diff of the 2 patch files. The only change of substance is that the protected static $modules in core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php was converted to a multi-line array in commit 6e594c26 for #3060603: Live preview is broken when editing the media_library view. Everything else is just context line numbers.

Given that, +1 to leaving this RTBC (although I haven't closely reviewed everything myself).

Cheers,
-Derek

review credits

Committed 006e179 and pushed to 8.8.x. Thanks!

c/p to 8.7.x