From 098f4353a7a50ced3b55acf569738731258cb30b Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Thu, 21 Sep 2023 16:17:05 +0200 Subject: [PATCH] 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 (https://github.com/TryGhost/Ghost/commit/1e3232cf8239787002c2865843cf8e0acf0e38ba) 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). --- .../core/core/server/lib/request-external.js | 14 +- .../test/e2e-server/services/mentions.test.js | 168 ++++++++++++++---- ghost/core/test/utils/overrides.js | 8 + .../webmentions/lib/MentionSendingService.js | 19 +- .../test/MentionSendingService.test.js | 22 +++ 5 files changed, 189 insertions(+), 42 deletions(-) diff --git a/ghost/core/core/server/lib/request-external.js b/ghost/core/core/server/lib/request-external.js index 2ce34e2228..56a7c51f1f 100644 --- 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); diff --git a/ghost/core/test/e2e-server/services/mentions.test.js b/ghost/core/test/e2e-server/services/mentions.test.js index 234f17077b..bcf6f55834 100644 --- 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 other site.`; +let mentionHtml2 = `Check out this really cool other site.`; let endpointUrl = new URL('https://www.endpoint.com/'); +let endpointUrl2 = new URL('https://www.endpoint2.com/'); let targetHtml = `Some content`; +let targetHtml2 = `Some content`; 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 () { diff --git a/ghost/core/test/utils/overrides.js b/ghost/core/test/utils/overrides.js index 50a29c7798..5502ef0443 100644 --- 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(); } diff --git a/ghost/webmentions/lib/MentionSendingService.js b/ghost/webmentions/lib/MentionSendingService.js index 7151cf68e3..43e1c1d00c 100644 --- 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); } } diff --git a/ghost/webmentions/test/MentionSendingService.test.js b/ghost/webmentions/test/MentionSendingService.test.js index 3daecab182..479d7024c1 100644 --- 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,