In reviewing other code, I have noticed that the "warnfix" parameter does not always work. We should add a test for this in both the D7 & D8 tests.

The test should create a node, open the URL node/1?amp&warnfix, and test for the following text:

AMP-HTML Validation Issues and Fixes
-------------------------------------
PASS

CommentFileSizeAuthor
#8 2745071-8-d7-warnfix-text.patch1.86 KBmtift
#8 2745071-8-d7-interdiff.txt893 bytesmtift
#4 2745071-4-d8-warnfix-text.patch2.4 KBmtift
#3 2745071-d7-warnfix-text.patch2 KBmtift
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mtift created an issue. See original summary.

mtift’s picture

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

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Active » Needs review
FileSize
2745071-d7-warnfix-text.patch2 KB

This issue revealed that our previous tests were not actually testing AMP pages, so I added a few other other things to test that the warnfix test is working (which, I'd say is a good thing).

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
FileSize
2745071-4-d8-warnfix-text.patch2.4 KB

And here's the D8 version.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented

Tested the D7 test. Tests are passing. Code review looks good.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented

I wonder about the bit about testing whether My Account is present on the non-amp vs amp page. That would depend on which blocks are placed in the theme. Is that something we need to test for the module?

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented

Tested the D8 version. You need to have composer_manager downloaded (but not necessarily enabled) for this test to work. That feels a little strange since we don't require Composer Manager for the module itself. Not sure what to do about that since presumably that is necessary for the AMP library to work with the test suite.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
FileSize
2745071-8-d7-interdiff.txt893 bytes
2745071-8-d7-warnfix-text.patch1.86 KB

Regarding the Drupal 7 version...

- I'm fine removing the My account tests, since that was mostly for testing the tests
- The tests pass with composer_manager and fail without it. So unless you have another suggestion, I'm going to leave that part in

Updated patches attached

mtift’s picture

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

And I guess there is nothing to change with the D8 patch

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Status: Needs review » Reviewed & tested by the community

Looks good ahead. Let's get these in.

  • mtift committed 2cd99f3 on 8.x-1.x 
    Issue #2745071 by mtift: Test warnfix parameter

  • mtift committed 9ad732a on 7.x-1.x 
    Issue #2745071 by mtift: Test warnfix parameter
mtift’s picture

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

Thanks, Marc. Pushed to 7.x-1.x and 8.x-1.x.

Status: Fixed » Closed (fixed)

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

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift at Lullabot commented