ampproject / ampbench

AMPBench: AMP URL validation and troubleshooting tools (DEPRECATED)
Apache License 2.0
66 stars 39 forks source link

AMPBench fails to validate structured data for ImageGallery #7

Closed poscar closed 7 years ago

poscar commented 8 years ago

Looks like the ampbench is failing to validate valid ld+json structured data for ImageGalleries. The goal is for us to get image gallery news content to show up in the carousel, but it seems as if we have errors that prevent this from happening.

The structured data for our AMP image galleries contains an array with two objects, one for NewsArticle and one for ImageGallery. According the structured data testing tool results this is perfectly valid. But, according to the ampbench results it is not.

I couldn't tell from the source code whether structured data with arrays are supported.

How come AMPBench validation fails for us? Is the AMPBench validation the same one Google uses for the Top News carousel?

Thanks!

pietergreyling commented 8 years ago

@poscar I will look into this - thanks for reporting!

pietergreyling commented 7 years ago

@poscar Can you verify that we are good now since the following commit? https://github.com/ampproject/ampbench/commit/efca953c87889bc4bca77000e90c9c6be3638739 Test URL: https://ampbench.appspot.com/validate?url=http://ap-stag.glamour.com/gallery/how-to-calm-stress/amp

poscar commented 7 years ago

@pietergreyling

I can confirm that it is now working for galleries, for example: https://ampbench.appspot.com/validate?url=http%3A%2F%2Fwww.glamour.com%2Fgallery%2Fbeauty-gift-ideas-sisters%2Famp

But now it is failing to validate regular NewsArticle here: https://ampbench.appspot.com/validate?url=http%3A%2F%2Fwww.glamour.com%2Fstory%2Fsupermodel-joan-smalls-has-gone-after-what-she-wants-in-life-glamour-october-2016%2Famp

It should be able to validate both. Thoughts?

pietergreyling commented 7 years ago

@poscar Will look into it and let you know.

FWIW, I see the following direct SDTT check for that page regularly times out (but not always and looks OK when not): https://search.google.com/structured-data/testing-tool#url=http://www.glamour.com/story/supermodel-joan-smalls-has-gone-after-what-she-wants-in-life-glamour-october-2016/amp https://search.google.com/structured-data/testing-tool#url=http%3A%2F%2Fwww.glamour.com%2Fstory%2Fsupermodel-joan-smalls-has-gone-after-what-she-wants-in-life-glamour-october-2016%2Famp

poscar commented 7 years ago

Ok, thanks @pietergreyling

Never seen it time-out, but will keep an eye out for that.

pietergreyling commented 7 years ago

Hi @poscar,

I just rolled out https://github.com/ampproject/ampbench/commit/1609cddfc65ab46cd65544438d7309199406e1ba and the handling for the following (+ others I tested) looks good: https://ampbench.appspot.com/validate?url=http://www.glamour.com/gallery/beauty-gift-ideas-sisters/amp https://ampbench.appspot.com/validate?url=http://www.glamour.com/story/supermodel-joan-smalls-has-gone-after-what-she-wants-in-life-glamour-october-2016/amp https://ampbench.appspot.com/validate?url=http://www.glamour.com/story/american-music-awards-2016-best-performances/amp https://ampbench.appspot.com/validate?url=http://www.glamour.com/story/e-kidman-and-hubby-keith-urban/amp

poscar commented 7 years ago

@pietergreyling

Perfect, this looks great! Thanks for looking into this and the fix!

Happy thanksgiving!