Write test for AMP formatter

This looks like a great idea!

Somewhat related: https://github.com/Lullabot/amp-library/issues/11 and https://github.com/Lullabot/amp-library/issues/52

Unless anyone else is working on this, I can take it.

This seems to be working. Feedback welcome.

Wrong patch

1. +++ b/src/Tests/AmpFormatterTest.php
   @@ -0,0 +1,111 @@
   +/**
   + * @file
   + * Contains \Drupal\amp\Tests\AmpFormatterTest.
   + */
   

   no longer needed.

2. +++ b/src/Tests/AmpFormatterTest.php
   @@ -0,0 +1,111 @@
   +/**
   + * Tests AMP view mode.
   + *
   + * @group amp
   + */
   +class AmpFormatterTest extends WebTestBase {
   

   we could also use browsertestbase or javascripttestbase to actually make sure the JS runs properly..

3. +++ b/src/Tests/AmpFormatterTest.php
   @@ -0,0 +1,111 @@
   +    'entity_test',
   

   what's entity test for? the module only supports node atm anyway.

Looks good as a basic test. Not sure how far you want to go here, adding test coverage for the warnfix stuff, which would ensue test coverage of cacheability there would for example be easy,

@Berdir, thanks for the review.

1. Removed
2. I'm not as familiar with BrowserTestBase, but it looks like those tests would be put in the modules/yourmodule/Tests/Functional directory, so perhaps that could be a follow-up? If you would be willing to open a follow-up issue with any specific ideas you had, I would be glad to do the work.
3. I was having problems with assigning proper permissions to view "admin/structure/types/manage/article/display" so using entity_test was kind of a hack. I think I have it worked out now.

Yes, BrowserTestBase is the phpunit+mink based alternative for WebTestBase. WebTestBase will eventually be removed, but first core has to convert its own many hundred web tests.

Not sure if a follow-up makes sense right now, either we'd create a BrowserTestBase from the start or wait until later when we have to convert it.

1. +++ b/src/Tests/AmpFormatterTest.php
   @@ -62,6 +54,14 @@ class AmpFormatterTest extends WebTestBase {
   +    // Create Article node type.
   +    if ($this->profile != 'standard') {
   

   it will never be standard, don't bother writing pseudo-generic code in a test. You know your environment :)

   I know some core tests do this, but that's more or less a left-over from when some tests were still using standard.

2. +++ b/src/Tests/AmpFormatterTest.php
   @@ -100,12 +97,14 @@ class AmpFormatterTest extends WebTestBase {
        $this->assertText('AMP test body');
        $this->assertRaw('data-quickedit-field-id="node/1/body/en/full"');
   +    $this->assertRaw('link rel="amphtml" href="/node/1?amp"');
   

   /node/1 will fail on testbot (which can't run this yet, but hopefully soon).

   Testbot uses a /checkout subfolder, so the path will be /checkout/node/1. You could build the URL instead of hardcoding it.

   See also the absolute path issue that I just opened..

3. +++ b/src/Tests/AmpFormatterTest.php
   @@ -100,12 +97,14 @@ class AmpFormatterTest extends WebTestBase {
        $this->drupalGet('node/1', ['query' => ['amp' => TRUE]]);
   

   what does that result in exactly, amp=1? if so, then I think it would be better to write just "?amp" into the query string.

   You could even try to extract it from the link attribute and try to follow that.

1. OK, I'll remove
2. Will do.
3. I think I tried putting "?amp" into the query string, and it failed, but I'll give it another go. 'amp' => TRUE is definitely sub-optimal since we don't do it that way in practice.

BrowserTestBase sounds like fun. I'll give it a go.

I can't figure out a better way to do this:

    $this->drupalGet('node/1', ['query' => ['amp' => TRUE]]);

However, the patch is now using BrowserTestBase and the tests are passing. So that's cool.

While I'm certain @Berdir knows how to run these tests, others might not because this is new stuff (see this change notice: https://www.drupal.org/node/2469723) with very few core examples. The way I have been testing is with a command like this:

sudo -u www-data php /var/www/amp/docroot/core/scripts/run-tests.sh --browser --url "http://amp.dev" --class "Drupal\amp\Functional\AmpFormatterTest"

I would recommend you run them directly with phpunit, e.g. like this:

... ./vendor/bin/phpunit -c core modules/amp

You can use --filter ClassOrMethodName to only run one test/method.

This gives you much better output in case of test fails.

You will have to set a few things, either as environment variables or in phpunit.xml, https://www.chapterthree.com/blog/javascript-testing-comes-to-drupal-8 has good description for that. (Parts of it applies to javascript test, but tests are run in the same way)

index 0000000..14b17e5
--- /dev/null

--- /dev/null
+++ b/src/Functional/AmpFormatterTest.php

I think this should be in the tests folder?

Yep. Moved back to Tests directory.

Let's fix the directory in the patch, too

That's not what I meant.

src/Tests => Simpletest tests (namespace: \Drupal\amp\Tests)
tests/src/(Functional|Unit|Kernel|FunctionalJavascriptTests) => phpunit tests (namespace: Drupal\Tests\amp\(Functional|...)

You had just src/Functional. Not sure why that even found your test :)

PS: I noticed you're always online in IRC but I think aren't really using. Tried to ping you a few times in the last week(s) :)

I think I finally figured out the proper directory structure. Then I was able to run the test with the following command:

cd /var/www/amp/docroot/; export SIMPLETEST_BASE_URL=mysql://user:pass@amp.dev; ./vendor/bin/phpunit -c core modules/contrib/amp

Looks good to me now, lets hope testbot adds support for composer soon :)

@Berdir, thanks for all of your help with this! Committed to 8.x-1.x.

Here's a Drupal 7 version. Not quite a full featured, but it's a start.

Extra blank link at end of amp.test.

Looks what we're testing in D8 is:
- Status code of 200 on the display modes page, which contains the text of AMP, which show that a content type has the AMP display mode available.
- Enable AMP display mode for that content type.
- Go to the node created for the test, check that quickedit is showing the view mode is full, and that there is an amphtml link.
- Load the AMP URL, check that quickedit is showing the view mode is amp, and that there is a canonical url pointing back to the main URL.

In D7:
- Checking the status code of the display modes page for a content type, but not checking that AMP is present.
- Enable AMP display mode for that content type.
- Go to the node created for the test and check that there is an amphtml link.
- Load the AMP URL, check that there is a canonical url pointing back to the main URL.

So in D7, we can't check the view mode with quickedit (since quickedit doesn't exist). It probably should be possible to do the check on the display mode page to see that AMP is a display mode option though.

Test code itself looks good. I ran the test, it seems to be functioning as intended.

Two thinks I'd look at:
- Extra line of whitespace in amp.test
- Checking that AMP text is available on display mode page.

I removed the blank line in the test file.

Since there is no "display mode page" (admin/structure/display-modes) page in Drupal 7, I did not change anything else.

Manually tested again. Two test failures, now:
- Raw "link rel="amphtml" href="/node/1?amp"" found
- Raw "link rel="canonical" href="/node/1"" found

My guess is this is because these URLs now have the domain name in them. Might be worth double-checking the test for D8 as well if the same URLs are being checked.

Good catch.

Code looks good, tested the test, that seems to run fine. RTBC.

Committed to 7.x-1.x. Thanks, Marc!