From f50224f445f6271ec313e33f492c2b86e9ef9ee7 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Tue, 7 Mar 2023 13:20:28 +0100 Subject: [PATCH] Improved network access and mocking in tests (#16371) refs https://github.com/TryGhost/Team/issues/2667 Some tests still accessed the internet. Now network access is disabled by default. This change also introduces two helper methods related to networking (mocking Slack and Mailgun). This fixes two unreliable tests: - Staff service was accessing a Slack test API -> timeout possible - MentionSendingService was trying to send webmentions for every post publish/change -> possible timeouts and job issues --- ghost/core/test/e2e-api/admin/oembed.test.js | 7 ++- .../test/e2e-server/services/mentions.test.js | 6 +-- .../email-service/batch-sending.test.js | 23 +++----- .../services/oembed/twitter-embed.test.js | 12 ++--- .../unit/server/services/staff/index.test.js | 1 + .../test/utils/e2e-framework-mock-manager.js | 54 ++++++++++++++++--- ghost/core/test/utils/e2e-framework.js | 3 ++ ghost/core/test/utils/overrides.js | 12 +++++ 8 files changed, 78 insertions(+), 40 deletions(-) diff --git a/ghost/core/test/e2e-api/admin/oembed.test.js b/ghost/core/test/e2e-api/admin/oembed.test.js index 6f0f2fca48..a063ff5b10 100644 --- a/ghost/core/test/e2e-api/admin/oembed.test.js +++ b/ghost/core/test/e2e-api/admin/oembed.test.js @@ -5,6 +5,7 @@ const supertest = require('supertest'); const testUtils = require('../../utils/index'); const config = require('../../../core/shared/config/index'); const localUtils = require('./utils'); +const {mockManager} = require('../../utils/e2e-framework'); // for sinon stubs const dnsPromises = require('dns').promises; @@ -20,9 +21,7 @@ describe('Oembed API', function () { beforeEach(function () { // ensure sure we're not network dependent - sinon.stub(dnsPromises, 'lookup').callsFake(function () { - return Promise.resolve({address: '123.123.123.123'}); - }); + mockManager.disableNetwork(); }); afterEach(function () { @@ -422,7 +421,7 @@ describe('Oembed API', function () { return Promise.resolve({address: '123.123.123.123'}); } }); - + const pageMock = nock('http://test.com') .get('/') .reply(200, ''); diff --git a/ghost/core/test/e2e-server/services/mentions.test.js b/ghost/core/test/e2e-server/services/mentions.test.js index 50fd80bc3c..b42c849231 100644 --- a/ghost/core/test/e2e-server/services/mentions.test.js +++ b/ghost/core/test/e2e-server/services/mentions.test.js @@ -1,9 +1,7 @@ const {agentProvider, fixtureManager, mockManager} = require('../../utils/e2e-framework'); -const sinon = require('sinon'); const nock = require('nock'); const assert = require('assert'); const markdownToMobiledoc = require('../../utils/fixtures/data-generator').markdownToMobiledoc; -const dnsPromises = require('dns').promises; const jobsService = require('../../../core/server/services/mentions-jobs'); let agent; @@ -30,9 +28,7 @@ describe('Mentions Service', function () { beforeEach(async function () { // externalRequest does dns lookup; stub to make sure we don't fail with fake domain names - sinon.stub(dnsPromises, 'lookup').callsFake(function () { - return Promise.resolve({address: '123.123.123.123'}); - }); + mockManager.disableNetwork(); // mock response from website mentioned by post to provide endpoint mentionMock = nock(mentionUrl.href) diff --git a/ghost/core/test/integration/services/email-service/batch-sending.test.js b/ghost/core/test/integration/services/email-service/batch-sending.test.js index 975cd9a3ef..50883e0fc6 100644 --- a/ghost/core/test/integration/services/email-service/batch-sending.test.js +++ b/ghost/core/test/integration/services/email-service/batch-sending.test.js @@ -181,9 +181,14 @@ describe('Batch sending tests', function () { stubbedSend = sinon.fake.resolves({ id: 'stubbed-email-id' }); + mockManager.mockMail(); + mockManager.mockMailgun(function () { + return stubbedSend.call(this, ...arguments); + }); }); afterEach(async function () { + mockManager.restore(); await configUtils.restore(); await models.Settings.edit([{ key: 'email_verification_required', @@ -192,21 +197,6 @@ describe('Batch sending tests', function () { }); before(async function () { - mockManager.mockSetting('mailgun_api_key', 'test'); - mockManager.mockSetting('mailgun_domain', 'example.com'); - mockManager.mockSetting('mailgun_base_url', 'test'); - mockManager.mockMail(); - - // We need to stub the Mailgun client before starting Ghost - sinon.stub(MailgunClient.prototype, 'getInstance').returns({ - // @ts-ignore - messages: { - create: async function () { - return await stubbedSend.call(this, ...arguments); - } - } - }); - const agents = await agentProvider.getAgentsWithFrontend(); agent = agents.adminAgent; frontendAgent = agents.frontendAgent; @@ -223,7 +213,6 @@ describe('Batch sending tests', function () { }); after(async function () { - mockManager.restore(); await ghostServer.stop(); }); @@ -240,7 +229,7 @@ describe('Batch sending tests', function () { await completedPromise; await emailModel.refresh(); - assert.equal(emailModel.get('status'), 'submitted'); + assert.equal(emailModel.get('status'), 'submitted', 'This email should be in submitted state: ' + (emailModel.get('error') ?? 'No error')); assert.equal(emailModel.get('email_count'), 4); // Did we create batches? diff --git a/ghost/core/test/unit/server/services/oembed/twitter-embed.test.js b/ghost/core/test/unit/server/services/oembed/twitter-embed.test.js index 9a0120d250..2307fb5856 100644 --- a/ghost/core/test/unit/server/services/oembed/twitter-embed.test.js +++ b/ghost/core/test/unit/server/services/oembed/twitter-embed.test.js @@ -3,6 +3,7 @@ const TwitterOEmbedProvider = require('../../../../../core/server/services/oembe const externalRequest = require('../../../../../core/server/lib/request-external'); const nock = require('nock'); const sinon = require('sinon'); +const {mockManager} = require('../../../../utils/e2e-framework'); const dnsPromises = require('dns').promises; describe('TwitterOEmbedProvider', function () { @@ -12,14 +13,11 @@ describe('TwitterOEmbedProvider', function () { beforeEach(function () { // external requests will attempt dns lookup - sinon.stub(dnsPromises, 'lookup').callsFake(function () { - return Promise.resolve({address: '123.123.123.123'}); - }); + mockManager.disableNetwork(); }); afterEach(async function () { - nock.cleanAll(); - sinon.restore(); + mockManager.restore(); }); after(async function () { @@ -47,8 +45,8 @@ describe('TwitterOEmbedProvider', function () { const tweetURL = new URL( 'https://twitter.com/Ghost/status/1630581157568839683' ); - - // not certain why we hit publish.twitter.com + + // not certain why we hit publish.twitter.com nock('https://publish.twitter.com') .get('/oembed') .query(true) diff --git a/ghost/core/test/unit/server/services/staff/index.test.js b/ghost/core/test/unit/server/services/staff/index.test.js index ae23bafad6..95b7b43ffa 100644 --- a/ghost/core/test/unit/server/services/staff/index.test.js +++ b/ghost/core/test/unit/server/services/staff/index.test.js @@ -18,6 +18,7 @@ describe('Staff Service:', function () { beforeEach(function () { mockManager.mockMail(); + mockManager.mockSlack(); userModelStub = sinon.stub(models.User, 'getEmailAlertUsers').resolves([{ email: 'owner@ghost.org', slug: 'ghost' diff --git a/ghost/core/test/utils/e2e-framework-mock-manager.js b/ghost/core/test/utils/e2e-framework-mock-manager.js index 4d913d0486..c67bb46c5f 100644 --- a/ghost/core/test/utils/e2e-framework-mock-manager.js +++ b/ghost/core/test/utils/e2e-framework-mock-manager.js @@ -2,6 +2,7 @@ const errors = require('@tryghost/errors'); const sinon = require('sinon'); const assert = require('assert'); const nock = require('nock'); +const MailgunClient = require('@tryghost/mailgun-client/lib/mailgun-client'); // Helper services const configUtils = require('./configUtils'); @@ -35,17 +36,31 @@ const disableStripe = async () => { await stripeService.disconnect(); }; -const mockStripe = () => { - nock.disableNetConnect(); -}; - const disableNetwork = () => { nock.disableNetConnect(); // externalRequest does dns lookup; stub to make sure we don't fail with fake domain names - sinon.stub(dnsPromises, 'lookup').callsFake(() => { - return Promise.resolve({address: '123.123.123.123', family: 4}); - }); + if (!dnsPromises.lookup.restore) { + sinon.stub(dnsPromises, 'lookup').callsFake(() => { + return Promise.resolve({address: '123.123.123.123', family: 4}); + }); + } + + // Allow localhost + nock.enableNetConnect('127.0.0.1'); +}; + +const mockStripe = () => { + disableNetwork(); +}; + +const mockSlack = () => { + disableNetwork(); + + nock(/hooks.slack.com/) + .persist() + .post('/') + .reply(200, 'ok'); }; /** @@ -68,6 +83,26 @@ const mockMail = (response = 'Mail is disabled') => { return mockMailReceiver; }; +const mockMailgun = (customStubbedSend) => { + mockSetting('mailgun_api_key', 'test'); + mockSetting('mailgun_domain', 'example.com'); + mockSetting('mailgun_base_url', 'test'); + + const stubbedSend = customStubbedSend ?? sinon.fake.resolves({ + id: `<${new Date().getTime()}.${0}.5817@samples.mailgun.org>` + }); + + // We need to stub the Mailgun client before starting Ghost + sinon.stub(MailgunClient.prototype, 'getInstance').returns({ + // @ts-ignore + messages: { + create: async function () { + return await stubbedSend.call(this, ...arguments); + } + } + }); +}; + const mockWebhookRequests = () => { mocks.webhookMockReceiver = new WebhookMockReceiver({snapshotManager}); @@ -210,6 +245,9 @@ const restore = () => { } mailService.GhostMailer.prototype.send = originalMailServiceSend; + + // Disable network again after restoring sinon + disableNetwork(); }; module.exports = { @@ -217,6 +255,8 @@ module.exports = { mockMail, disableStripe, mockStripe, + mockSlack, + mockMailgun, mockLabsEnabled, mockLabsDisabled, mockWebhookRequests, diff --git a/ghost/core/test/utils/e2e-framework.js b/ghost/core/test/utils/e2e-framework.js index 5e4344afd0..334263bfca 100644 --- a/ghost/core/test/utils/e2e-framework.js +++ b/ghost/core/test/utils/e2e-framework.js @@ -82,6 +82,9 @@ const startGhost = async (options = {}) => { await urlServiceUtils.isFinished(); } + // Disable network in tests at the start + mockManager.disableNetwork(); + return ghostServer; }; diff --git a/ghost/core/test/utils/overrides.js b/ghost/core/test/utils/overrides.js index 3a1cd66880..cfbc6f9220 100644 --- a/ghost/core/test/utils/overrides.js +++ b/ghost/core/test/utils/overrides.js @@ -5,3 +5,15 @@ require('../../core/server/overrides'); const {mochaHooks} = require('@tryghost/express-test').snapshot; exports.mochaHooks = mochaHooks; + +const mockManager = require('./e2e-framework-mock-manager'); + +const originalBeforeAll = mochaHooks.beforeAll; +mochaHooks.beforeAll = async function () { + if (originalBeforeAll) { + await originalBeforeAll(); + } + + // Disable network in tests to prevent any accidental requests + mockManager.disableNetwork(); +};