#2717641: Add support for 'Top Stories with AMP' adds new a new configuration entity for AMP Metadata. We should have tests to make sure it does what it is supposed to do, which is as follows:

The first config entity created provides global settings for AMP Metadata.

Config entities added when there is already global settings allow selection of a content type to provide a settings override just for that content type. Only content types that are AMP-enabled, and which do not already have a content type override, will be able to be selected.

The config entities have two main types of settings: settings for the organization and for individual pieces of content.

Organization:
- Organization name
- Organization logo
- Image style for organization logo

Content:
- Token for node author
- Token for node description
- Token for image for a node
- Image style to apply to content image

The author and description tokens selected should ideally not return HTML, and the description should return fewer than 150 characters. Not sure there is a way to require that, though. The image token should return an FID ideally. Again, hard to require that.

On a content type override config entity, the organization fields are collapsed by default. While it's possible to set those per content type, it's likely those will be set globally.

CommentFileSizeAuthor
#6 2737641-6-test-metadata.patch4.98 KBmtift
#5 2737641-5-test-metadata.patch3.22 KBmtift

Comments

mdrummond created an issue. See original summary.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Assigned: Unassigned » mtift
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Issue tags: +1.0 blocker
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented

Because we need to upload files to test metadata, this issue will require a change in Drupal core: #2738567: Add test trait for drupalGetTestFiles and drupalCompareFile.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Status: Active » Needs work
StatusFileSize
new 2737641-5-test-metadata.patch3.22 KB

I'm not exactly sure this is all going to work, but I'm getting closer. The attached patch depends on my patch from #2738567: Add test trait for drupalGetTestFiles and drupalCompareFile. There are at least two things that I could not get quite fully get working: (1) tokens and (2) images.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2737641-6-test-metadata.patch4.98 KB

Well, I guess there is no reason to get too crazy with these tests. This patch tests most of the metadata in the script type="application/ld+json" section. It does *not* test that image styles are applied because I'm not sure that's necessarily the responsibility of the AMP module. It does test that the image width and height appear in the metadata. I'm also not sure it's worth the effort to figure out how to apply those image styles in a test that extends BrowserTestBase.

Finally, these tests only pass with the patch applied from #2738567-5: Add test trait for drupalGetTestFiles and drupalCompareFile, so I guess we'll have to decide what to do about that. We will probably need to postpone this issue on that one. We could just temporarily grab the code from that patch and use it in the AMP module (it's just one file/trait), but I'm not sure this issue rises to that level of urgency.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented
Status: Needs review » Needs work

Applied the test and ran it, seems to work fine. Two notes from reviewing the test:

1) Might be worthwhile adding metadata settings for the article node type to check that the merging of metadata works correctly.

2) It looks like this is using the same image for the organization logo and the content logo. Might be worth checking those separately. Given how this is being checked, I can imagine that might be difficult since both would have the same @ImageObject, height and width properties. Might require an xpath selection?

If it proves too difficult, probably don't need to get too into the weeds for point two. This provides some nice testing and would be great to get this in. Can always make further improvements down the road.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Issue tags: -1.0 blocker

I don't think we should let this block the 1.0 release.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented
Status: Needs work » Reviewed & tested by the community
Related issues: +#2758751: Expand tests for AMP Metadata settings

After thinking about this some more, let's get these tests in now and expand on them in another issue: #2758751: Expand tests for AMP Metadata settings

Patch still applies. Let's get this in.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Status: Reviewed & tested by the community » Postponed

Thanks, Marc.

This is blocked on #2738567: Add test trait for drupalGetTestFiles and drupalCompareFile

karens’s picture

karens commented
Status: Postponed » Fixed

Not necessary now. See the new 8.2 branch, which uses the Schema.org Metatag module. That module has tests.

karens’s picture

karens commented
Status: Fixed » Closed (won't fix)