Make WordPress Core

Opened 9 years ago

Closed 7 years ago

#32360 closed enhancement (maybelater)

Add oEmbed provider unit tests

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: Priority: normal
Severity: normal Version:
Component: Embeds Keywords: needs-patch
Focuses: Cc:

Description

A unit test should be added which tests each of our supported oEmbed providers to make sure that each provider returns the expected response.

Ideally the tests would be in their own group which is excluded from a default test run, in the same way AJAX tests work. The tests should test the HTTP response code and the validity of the JSON and XML endpoints where appropriate.

Attachments (3)

32360.patch (15.0 KB) - added by johnbillion 9 years ago.
32360.2.patch (20.0 KB) - added by johnbillion 8 years ago.
32360.diff (25.8 KB) - added by wonderboymusic 8 years ago.

Download all attachments as: .zip

Change History (24)

#1 @voldemortensen
9 years ago

I'll start working on this tomorrow.

#2 @johnbillion
9 years ago

Thanks voldemortensen. If you need media URLs for any of the providers, #28507 should have one for each.

@johnbillion
9 years ago

#3 @johnbillion
9 years ago

  • Component changed from Build/Test Tools to Embeds
  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.4

32360.patch introduces tests for the responses returned by each of our oEmbed providers. The tests which actually request the oEmbed for each of the test URLs is in the external-oembed group which doesn't run by default (these can be run with the --group=oembed or --group=external-oembed CLI option).

Currently missing from the tests:

  • Test URL(s) for poll.fm.
  • Test URL(s) for video214.com.
  • Test URL(s) for gi*.photobucket.com (*cough* #28379 *cough*).

Currently failing:

  • scribd.com is missing the width attribute.
  • polldaddy.com is missing the width attribute.
  • meetup.com returns an incorrect content-type header.
  • animoto.com returns an incorrect content-type header.
  • tumblr.com doesn't respect the maxwidth parameter for its width attribute.
  • cloudup.com doesn't respect the maxwidth parameter for its thumbnail_width attribute.
  • reverbnation.com doesn't adhere to the oEmbed specification (see #33207).

#4 @johnbillion
9 years ago

Other things I'd like to test:

  • Check the correct URL scheme is used in the response when https URLs are requested.
  • Check invalid maxwidth and maxheight parameters are handled correctly (background: 29126#comment:9).
  • Check HTTPS support? (See #28507)

#5 @wonderboymusic
9 years ago

  • Owner set to johnbillion
  • Status changed from new to assigned

@johnbillion
8 years ago

#6 @johnbillion
8 years ago

  • Keywords ongoing added
  • Milestone changed from 4.4 to Future Release

Latest patch, 32360.2.patch, tests everything I want to cover. Twelve failures. Bleugh.

#7 @netweb
8 years ago

Related: #34651 Instagram URLs with www will not embed

With that, the tests here for Instagram should be updated to include the w's per r35634

@wonderboymusic
8 years ago

#8 @wonderboymusic
8 years ago

  • Keywords ongoing removed
  • Milestone changed from Future Release to 4.7

#9 @wonderboymusic
8 years ago

In 38454:

OEmbed: add unit tests. @group external-oembed is not run by default.

Props johnbillion, wonderboymusic.
See #32360.

#10 @jorbin
8 years ago

@wonderboymusic @johnbillion Did you intend for the grunt task to be commented out? If so, why add it?

#11 @johnbillion
8 years ago

  • Keywords needs-patch added; has-patch removed

The testOembedTestsCoverAllProviders() test has been altered from my patch and now doesn't work as intended. It should fail when a new provider is added to WP_oEmbed->providers, which it doesn't.

#12 @johnbillion
8 years ago

In 38512:

Embeds: Update the oEmbed provider test suite.

  • Remove the manual flag for HTTPS support and replace it with a simple check on the URL format.
  • Ensure testOembedTestsCoverAllProviders() actually fails when a new provider is added without a corresponding test.

See #32360

#13 @johnbillion
8 years ago

So the reason these tests hadn't gone into core up until this point is because there are so many failures with the external providers.

The tests are quite volatile too (I can see at least two tested URLs which are 404ing that previously weren't), for example Kickstarter is sending me 429 Too Many Requests responses, and Cloudup is sending me 502 Bad Gateway intermittently.

I guess it makes sense for the tests to remain in a group which never gets tested by default (ie. without passing the --group external-oembed flag), but then I'm not sure if that's super valuable.

In addition, the testOembedProviderReturnsExpectedResponse() test really needs to be split into several smaller tests so that every assertion can be performed (as opposed to the test failing at the first assertion failure). This means moving the actual HTTP requests into a data provider.

Last edited 8 years ago by johnbillion (previous) (diff)

#14 @johnbillion
8 years ago

In 38514:

Embeds: Clarify some assertion failure messages and correct a test URL for Twitter timelines.

See #32360

#15 @boonebgorges
8 years ago

I guess it makes sense for the tests to remain in a group which never gets tested by default (ie. without passing the --group external-oembed flag), but then I'm not sure if that's super valuable.

I just ran into this, because the <exclude><group>external-oembed</group></exclude> added to phpunit.xml.dist wasn't automatically picked up by my custom phpunit.xml.

I definitely think it's wise to have tests for this stuff, and I think it's smart for them not to run by default. But if they're failing on a regular and predictable basis, then they shouldn't be in core (or perhaps they should be commented out in the dataProvider while the details are ironed out).

#16 @ocean90
7 years ago

In 38692:

Embeds: Update the oEmbed provider test suite.

  • Dailymotion: Use an URL for an existing video.
  • Facebook: Remove URLs which don't exist and update provider map for [38691].

See #32360.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


7 years ago

#19 @johnbillion
7 years ago

In 38840:

oEmbed: Remove the oEmbed provider unit tests.

This reverts [38454] along with its follow-up commits, [38512], [38514], and [38692]. These tests are currently not pass
ing, and maybe they never will. The tests are in a group which does not run by default without a flag, making them quest
ionably useful.

We can re-visit this at a later date.

See #32360

#20 @johnbillion
7 years ago

  • Milestone changed from 4.7 to Future Release

#21 @johnbillion
7 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.