Disabled network retries for webmentions in tests (#18269)

refs https://ghost.slack.com/archives/C02G9E68C/p1695296293667689 - We block all outgoing networking by default in tests. When editing a post/page, Ghost tries to send a webmention. Because of my earlier changes (1e3232cf82) it tries 3 times - all network requests fail instantly when networking is disabled - but it adds a delay in between those retries. So my change disables retries during tests. - Adds a warning when afterEach hook tooks longer than 2s due to awaiting jobs/events - We reduce the amount of webmentions that are send, by only sending webmentions for added or removed urls, no longer when a post html is changed (unless post is published/removed).
2023-09-21 16:17:05 +02:00 · 2023-09-21 16:17:05 +02:00 · 098f4353a7
commit 098f4353a7
parent 16346dfc97
5 changed files with 189 additions and 42 deletions
--- a/ghost/core/core/server/lib/request-external.js
+++ b/ghost/core/core/server/lib/request-external.js
@ -46,6 +46,15 @@ async function errorIfInvalidUrl(options) {
    }
 }

+async function disableRetries(options) {
+    // Force disable retries
+    options.retry = {
+        limit: 0,
+        calculateDelay: () => 0
+    };
+    options.timeout = 5000;
+}
+
 // same as our normal request lib but if any request in a redirect chain resolves
 // to a private IP address it will be blocked before the request is made.
 const gotOpts = {
@ -54,13 +63,10 @@ const gotOpts = {
    },
    timeout: 10000, // default is no timeout
    hooks: {
+        init: process.env.NODE_ENV?.startsWith('test') ? [disableRetries] : [],
        beforeRequest: [errorIfInvalidUrl, errorIfHostnameResolvesToPrivateIp],
        beforeRedirect: [errorIfHostnameResolvesToPrivateIp]
    }
 };

-if (process.env.NODE_ENV?.startsWith('test')) {
-    gotOpts.retry = 0;
-}
-
 module.exports = got.extend(gotOpts);
--- a/ghost/core/test/e2e-server/services/mentions.test.js
+++ b/ghost/core/test/e2e-server/services/mentions.test.js
@ -6,9 +6,13 @@ const jobsService = require('../../../core/server/services/mentions-jobs');

 let agent;
 let mentionUrl = new URL('https://www.otherghostsite.com/');
+let mentionUrl2 = new URL('https://www.otherghostsite2.com/');
 let mentionHtml = `Check out this really cool <a href="${mentionUrl.href}">other site</a>.`;
+let mentionHtml2 = `Check out this really cool <a href="${mentionUrl2.href}">other site</a>.`;
 let endpointUrl = new URL('https://www.endpoint.com/');
+let endpointUrl2 = new URL('https://www.endpoint2.com/');
 let targetHtml = `<head><link rel="webmention" href="${endpointUrl.href}"</head><body>Some content</body>`;
+let targetHtml2 = `<head><link rel="webmention" href="${endpointUrl2.href}"</head><body>Some content</body>`;
 let mentionMock;
 let endpointMock;
 const DomainEvents = require('@tryghost/domain-events');
@ -18,6 +22,25 @@ const mentionsPost = {
    mobiledoc: markdownToMobiledoc(mentionHtml)
 };

+const editedMentionsPost = {
+    title: 'testing sending webmentions',
+    mobiledoc: markdownToMobiledoc(mentionHtml2)
+};
+
+function addMentionMocks() {
+    // mock response from website mentioned by post to provide endpoint
+    mentionMock = nock(mentionUrl.href)
+        .persist()
+        .get('/')
+        .reply(200, targetHtml, {'content-type': 'text/html'});
+
+    // mock response from mention endpoint, usually 201, sometimes 202
+    endpointMock = nock(endpointUrl.href)
+        .persist()
+        .post('/')
+        .reply(201);
+}
+
 describe('Mentions Service', function () {
    before(async function () {
        agent = await agentProvider.getAdminAPIAgent();
@ -30,16 +53,7 @@ describe('Mentions Service', function () {
        mockManager.disableNetwork();

        // mock response from website mentioned by post to provide endpoint
-        mentionMock = nock(mentionUrl.href)
-            .persist()
-            .get('/')
-            .reply(200, targetHtml, {'content-type': 'text/html'});
-
-        // mock response from mention endpoint, usually 201, sometimes 202
-        endpointMock = nock(endpointUrl.href)
-            .persist()
-            .post('/')
-            .reply(201);
+        addMentionMocks();

        await jobsService.allSettled();
        await DomainEvents.allSettled();
@ -123,7 +137,7 @@ describe('Mentions Service', function () {
                assert.equal(endpointMock.isDone(), true);
            });

-            it('Edited published post (post.published.edited)', async function () {
+            it('Does not send for edited post without url changes (post.published.edited)', async function () {
                const publishedPost = {status: 'published', ...mentionsPost};
                const res = await agent
                    .post('posts/')
@ -138,17 +152,9 @@ describe('Mentions Service', function () {
                assert.equal(endpointMock.isDone(), true);

                nock.cleanAll();
-
-                // reset mocks for mention
-                const mentionMockTwo = nock(mentionUrl.href)
-                    .persist()
-                    .get('/')
-                    .reply(200, targetHtml, {'content-type': 'text/html'});
-
-                const endpointMockTwo = nock(endpointUrl.href)
-                    .persist()
-                    .post('/')
-                    .reply(201);
+                addMentionMocks();
+                assert.equal(mentionMock.isDone(), false, 'should be reset');
+                assert.equal(endpointMock.isDone(), false, 'should be reset');

                const postId = res.body.posts[0].id;
                const editedPost = {
@ -163,8 +169,59 @@ describe('Mentions Service', function () {
                await jobsService.allSettled();
                await DomainEvents.allSettled();

+                assert.equal(mentionMock.isDone(), false);
+                assert.equal(endpointMock.isDone(), false);
+            });
+
+            it('Does send for edited post with url changes (post.published.edited)', async function () {
+                const publishedPost = {status: 'published', ...mentionsPost};
+                const res = await agent
+                    .post('posts/')
+                    .body({posts: [publishedPost]})
+                    .expectStatus(201);
+
+                await jobsService.allSettled();
+                await DomainEvents.allSettled();
+
+                // while not the point of the test, we should have real links/mentions to start with
+                assert.equal(mentionMock.isDone(), true);
+                assert.equal(endpointMock.isDone(), true);
+
+                nock.cleanAll();
+                addMentionMocks();
+                assert.equal(mentionMock.isDone(), false, 'should be reset');
+                assert.equal(endpointMock.isDone(), false, 'should be reset');
+
+                // reset mocks for mention
+                const mentionMockTwo = nock(mentionUrl2.href)
+                    .persist()
+                    .get('/')
+                    .reply(200, targetHtml2, {'content-type': 'text/html'});
+
+                const endpointMockTwo = nock(endpointUrl2.href)
+                    .persist()
+                    .post('/')
+                    .reply(201);
+
+                const postId = res.body.posts[0].id;
+                const editedPost = {
+                    ...editedMentionsPost,
+                    updated_at: res.body.posts[0].updated_at
+                };
+
+                await agent.put(`posts/${postId}/`)
+                    .body({posts: [editedPost]})
+                    .expectStatus(200);
+
+                await jobsService.allSettled();
+                await DomainEvents.allSettled();
+
                assert.equal(mentionMockTwo.isDone(), true);
                assert.equal(endpointMockTwo.isDone(), true);
+
+                // Also send again to the deleted url
+                assert.equal(mentionMock.isDone(), true);
+                assert.equal(endpointMock.isDone(), true);
            });

            it('Unpublished post (post.unpublished)', async function () {
@ -224,7 +281,7 @@ describe('Mentions Service', function () {
                assert.equal(endpointMock.isDone(), true);
            });

-            it('Edited published page (page.published.edited)', async function () {
+            it('Edited published page without url changes (page.published.edited)', async function () {
                const publishedPage = {status: 'published', ...mentionsPost};
                const res = await agent
                    .post('pages/')
@ -239,17 +296,9 @@ describe('Mentions Service', function () {
                assert.equal(endpointMock.isDone(), true);

                nock.cleanAll();
-
-                // reset mocks for mention
-                const mentionMockTwo = nock(mentionUrl.href)
-                    .persist()
-                    .get('/')
-                    .reply(200, targetHtml, {'content-type': 'text/html'});
-
-                const endpointMockTwo = nock(endpointUrl.href)
-                    .persist()
-                    .post('/')
-                    .reply(201);
+                addMentionMocks();
+                assert.equal(mentionMock.isDone(), false, 'should be reset');
+                assert.equal(endpointMock.isDone(), false, 'should be reset');

                const pageId = res.body.pages[0].id;
                const editedPage = {
@ -264,8 +313,59 @@ describe('Mentions Service', function () {
                await jobsService.allSettled();
                await DomainEvents.allSettled();

+                assert.equal(mentionMock.isDone(), false);
+                assert.equal(mentionMock.isDone(), false);
+            });
+
+            it('Edited published page with url changes (page.published.edited)', async function () {
+                const publishedPage = {status: 'published', ...mentionsPost};
+                const res = await agent
+                    .post('pages/')
+                    .body({pages: [publishedPage]})
+                    .expectStatus(201);
+
+                await jobsService.allSettled();
+                await DomainEvents.allSettled();
+
+                // while not the point of the test, we should have real links/mentions to start with
+                assert.equal(mentionMock.isDone(), true);
+                assert.equal(endpointMock.isDone(), true);
+
+                nock.cleanAll();
+                addMentionMocks();
+                assert.equal(mentionMock.isDone(), false, 'should be reset');
+                assert.equal(endpointMock.isDone(), false, 'should be reset');
+
+                // reset mocks for mention
+                const mentionMockTwo = nock(mentionUrl2.href)
+                    .persist()
+                    .get('/')
+                    .reply(200, targetHtml2, {'content-type': 'text/html'});
+
+                const endpointMockTwo = nock(endpointUrl2.href)
+                    .persist()
+                    .post('/')
+                    .reply(201);
+
+                const pageId = res.body.pages[0].id;
+                const editedPage = {
+                    ...editedMentionsPost,
+                    updated_at: res.body.pages[0].updated_at
+                };
+
+                await agent.put(`pages/${pageId}/`)
+                    .body({pages: [editedPage]})
+                    .expectStatus(200);
+
+                await jobsService.allSettled();
+                await DomainEvents.allSettled();
+
                assert.equal(mentionMockTwo.isDone(), true);
                assert.equal(endpointMockTwo.isDone(), true);
+
+                // Also send again to the deleted url
+                assert.equal(mentionMock.isDone(), true);
+                assert.equal(endpointMock.isDone(), true);
            });

            it('Unpublished post (post.unpublished)', async function () {
--- a/ghost/core/test/utils/overrides.js
+++ b/ghost/core/test/utils/overrides.js
@ -6,6 +6,7 @@ require('../../core/server/overrides');
 const {mochaHooks} = require('@tryghost/express-test').snapshot;
 exports.mochaHooks = mochaHooks;

+const chalk = require('chalk');
 const mockManager = require('./e2e-framework-mock-manager');

 const originalBeforeAll = mochaHooks.beforeAll;
@ -24,6 +25,11 @@ mochaHooks.afterEach = async function () {
    const mentionsJobsService = require('../../core/server/services/mentions-jobs');
    const jobsService = require('../../core/server/services/jobs');

+    let timeout = setTimeout(() => {
+        // eslint-disable-next-line no-console
+        console.error(chalk.yellow('\n[SLOW TEST] It takes longer than 2s to wait for all jobs and events to settle in the afterEach hook\n'));
+    }, 2000);
+
    await domainEvents.allSettled();
    await mentionsJobsService.allSettled();
    await jobsService.allSettled();
@ -31,6 +37,8 @@ mochaHooks.afterEach = async function () {
    // Last time for events emitted during jobs
    await domainEvents.allSettled();

+    clearTimeout(timeout);
+
    if (originalAfterEach) {
        await originalAfterEach();
    }
--- a/ghost/webmentions/lib/MentionSendingService.js
+++ b/ghost/webmentions/lib/MentionSendingService.js
@ -63,7 +63,7 @@ module.exports = class MentionSendingService {
                return;
            }
            // make sure we have something to parse before we create a job
-            let html = post.get('html');
+            let html = post.get('status') === 'published' ? post.get('html') : null;
            let previousHtml = post.previous('status') === 'published' ? post.previous('html') : null;
            if (html || previousHtml) {
                await this.#jobService.addJob('sendWebmentions', async () => {
@ -122,12 +122,23 @@ module.exports = class MentionSendingService {
     * @param {string|null} [resource.previousHtml]
     */
    async sendForHTMLResource(resource) {
-        const links = resource.html ? this.getLinks(resource.html) : [];
+        let links = resource.html ? this.getLinks(resource.html) : [];
        if (resource.previousHtml) {
-            // We also need to send webmentions for removed links
+            // Only send for NEW or DELETED links (to avoid spamming when lots of small changes happen to a post)
+            const existingLinks = links;
+            links = [];
            const oldLinks = this.getLinks(resource.previousHtml);
+
            for (const link of oldLinks) {
-                if (!links.find(l => l.href === link.href)) {
+                if (!existingLinks.find(l => l.href === link.href)) {
+                    // Deleted link
+                    links.push(link);
+                }
+            }
+
+            for (const link of existingLinks) {
+                if (!oldLinks.find(l => l.href === link.href)) {
+                    // New link
                    links.push(link);
                }
            }
--- a/ghost/webmentions/test/MentionSendingService.test.js
+++ b/ghost/webmentions/test/MentionSendingService.test.js
@ -150,6 +150,28 @@ describe('MentionSendingService', function () {
            assert.equal(firstCall.previousHtml, null);
        });

+        it('Sends on unpublish', async function () {
+            const service = new MentionSendingService({
+                isEnabled: () => true,
+                getPostUrl: () => 'https://site.com/post/',
+                jobService: jobService
+            });
+            const stub = sinon.stub(service, 'sendForHTMLResource');
+            await service.sendForPost(createModel({
+                status: 'draft',
+                html: 'same',
+                previous: {
+                    status: 'published',
+                    html: 'same'
+                }
+            }));
+            sinon.assert.calledOnce(stub);
+            const firstCall = stub.getCall(0).args[0];
+            assert.equal(firstCall.url.toString(), 'https://site.com/post/');
+            assert.equal(firstCall.html, null);
+            assert.equal(firstCall.previousHtml, 'same');
+        });
+
        it('Sends on html change', async function () {
            const service = new MentionSendingService({
                isEnabled: () => true,