From af63b8a39dcb790483a7274109b666ec1dd3bdd1 Mon Sep 17 00:00:00 2001 From: Dominique Hazael-Massieux Date: Fri, 13 Oct 2023 13:23:11 +0200 Subject: [PATCH 1/2] Take into account spec generation time when comparing PR and spec date see also #11 --- gh-webhook.js | 41 +++++++++++++++++++++++++---------------- package.json | 1 + 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/gh-webhook.js b/gh-webhook.js index 19a962a..a73442a 100644 --- a/gh-webhook.js +++ b/gh-webhook.js @@ -2,6 +2,10 @@ const {Octokit} = require("@octokit/rest"); const http = require('http'); const { Webhooks, createNodeMiddleware } = require("@octokit/webhooks"); +const { Temporal, toTemporalInstant } = require("@js-temporal/polyfill"); +Date.prototype.toTemporalInstant = toTemporalInstant; + + const {listRemovedTargets} = require("./lib/list-removed-targets"); const Github = require("./lib/github"); const {formatReport} = require("./lib/format-report"); @@ -35,13 +39,18 @@ function serve(GH_TOKEN, GH_SECRET, PORT, WEBREF_PATH, experimental) { return; } - // skip the event if the pull request is based on a commit older - // than the current version of the spec, to avoid false positives - const prDate = await github.getBaseCommitDate(payload.repository.full_name, payload.pull_request.base.sha); - if (spec?.crawlCacheInfo?.lastModified && JSON.stringify(prDate) < JSON.stringify(new Date(spec.crawlCacheInfo.lastModified))) { - return; + // skip the event if the pull request is based on a commit 15 minutes older + // than the current version of the spec, to avoid false positives (while + // taking into account the delay between a commit and the last-modified + // date due to spec generation time) + if (spec?.crawlCacheInfo?.lastModified) { + const prDate = await github.getBaseCommitDate(payload.repository.full_name, payload.pull_request.base.sha); + const lmDate = (new Date(spec.crawlCacheInfo.lastModified)).toTemporalInstant(); + // 15 minutes buffer to account for spec generation + if (lmDate.since(prDate, { largestUnit: 'minute'} ).minutes > 15) { + return; + } } - targets = await listRemovedTargets(spec, WEBREF_PATH); } catch (err) { console.error("Failed to process " + JSON.stringify(payload, null, 2)); @@ -49,17 +58,17 @@ function serve(GH_TOKEN, GH_SECRET, PORT, WEBREF_PATH, experimental) { throw(err); } - const existingReport = await github.findReport(payload.repository.full_name, payload.pull_request.number, "removedtargets"); - if (!existingReport || experimental) { - if (targets.length) { - if (experimental) { - const [repoFullname, issueNumber] = experimental.split("#"); - await github.postComment(repoFullname, issueNumber, `[${payload.repository.full_name}#${payload.pull_request.number}](${payload.pull_request.html_url}):\n${formatReport(targets, spec)}`); - } else { + if (targets.length) { + if (experimental) { + const [repoFullname, issueNumber] = experimental.split("#"); + await github.postComment(repoFullname, issueNumber, `[${payload.repository.full_name}#${payload.pull_request.number}](${payload.pull_request.html_url}):\n${formatReport(targets, spec)}`); + } else { + const existingReport = await github.findReport(payload.repository.full_name, payload.pull_request.number, "removedtargets"); + if (!existingReport) { await github.postComment(payload.repository.full_name, payload.pull_request.number, formatReport(targets, spec)); - } - } - } // TODO: update report if one exists and its content differs? + } // TODO: update report if one exists and its content differs? + } + } }); return server; diff --git a/package.json b/package.json index f3c438d..16dd42f 100644 --- a/package.json +++ b/package.json @@ -1,5 +1,6 @@ { "dependencies": { + "@js-temporal/polyfill": "^0.4.4", "@octokit/rest": "^19.0.7", "@octokit/webhooks": "^11.0.0", "http-server": "^14.1.1", From eb642f10e152c9e42173e0608bd66b5ce07fe595 Mon Sep 17 00:00:00 2001 From: Dominique Hazael-Massieux Date: Fri, 13 Oct 2023 16:33:33 +0200 Subject: [PATCH 2/2] Remove nock now that reffy uses undici fetch --- .github/workflows/test.yml | 4 +-- package.json | 3 +-- test/gh-webhook.js | 51 +++++++++++++++++++++++++------------- test/github.js | 8 +++--- 4 files changed, 42 insertions(+), 24 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3df7081..8d3d1e6 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -15,10 +15,10 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout repository - uses: actions/checkout@v2 + uses: actions/checkout@v3 - name: Setup Node.js - uses: actions/setup-node@v2 + uses: actions/setup-node@v3 with: node-version: '18' diff --git a/package.json b/package.json index 16dd42f..03bcfb0 100644 --- a/package.json +++ b/package.json @@ -5,11 +5,10 @@ "@octokit/webhooks": "^11.0.0", "http-server": "^14.1.1", "jsdom": "^22.0.0", - "reffy": "^13.0.0" + "reffy": "^14.1.0" }, "devDependencies": { "mocha": "^10.2.0", - "nock": "^13.3.1", "supertest": "^6.3.3", "undici": "^5.22.0" }, diff --git a/test/gh-webhook.js b/test/gh-webhook.js index a2c6833..1a3470e 100644 --- a/test/gh-webhook.js +++ b/test/gh-webhook.js @@ -5,16 +5,15 @@ const assert = require("assert"); const fs = require("fs").promises; -const { MockAgent, setGlobalDispatcher } = require('undici'); -// We need both undici.MockAgent and nock -// because reffy doesn't use node's fetch yet -const nock = require("nock"); +const { MockAgent, setGlobalDispatcher, getGlobalDispatcher } = require('undici'); const request = require('supertest'); const GhMock = require("./gh-api-mock"); const { formatReport } = require("../lib/format-report"); const { baseDirUrl } = require("../lib/util"); const webhook = require("../gh-webhook"); +const origDispatcher = getGlobalDispatcher(); + const agent = new MockAgent(); const ghMock = new GhMock(agent); @@ -50,15 +49,15 @@ function mockPreviewSpec(link) { if (cache[link]) return; cache[link] = true; if (link === testPreviewLink) { - nock('https://pr-preview.s3.amazonaws.com') - .get('/' + link.split('/').slice(3).join('/')) + agent.get('https://pr-preview.s3.amazonaws.com') + .intercept({method: "GET", path: '/' + link.split('/').slice(3).join('/')}) .reply(200, mockSpecs[link], { headers: {"Content-Type": "text/html; charset=utf-8"} }); return; } else if (link === testWhatwgMultiPreviewIndex) { const basePath = '/' + baseDirUrl(testWhatwgMultiPreviewIndex).split('/').slice(3).join('/'); - const scope = nock('https://whatpr.org'); + const client = agent.get('https://whatpr.org'); for (const page of ["subpage.html", "subpage2.html"]) { - scope.get(basePath + page) + client.intercept({method: "GET", path: basePath + page}) .reply(200, mockSpecs[baseDirUrl(testWhatwgMultiPreviewIndex) + page]); } } @@ -109,6 +108,7 @@ const removedTargets = (links) => { }]; }; +const isOK = res => res.status === 200 || res.status === 202; describe("the webhook server", function() { this.timeout(20000); @@ -132,7 +132,7 @@ describe("the webhook server", function() { mockPreviewSpec(testPreviewLink); const payload = editPrPayload("pr-preview[bot]", repo); try { - const res = await setupRequest(req, payload).expect(200); + const res = await setupRequest(req, payload).expect(isOK); } catch (err) { assert(false, err); } @@ -147,7 +147,7 @@ describe("the webhook server", function() { ghMock.listComments(repo, prNumber, [matchingComment]); const payload = editPrPayload("pr-preview[bot]", repo); try { - const res = await setupRequest(req, payload).expect(200); + const res = await setupRequest(req, payload).expect(isOK); } catch (err) { assert(false, err); } @@ -164,7 +164,7 @@ describe("the webhook server", function() { mockPreviewSpec(testWhatwgMultiPreviewIndex); const payload = editPrPayload("pr-preview[bot]", repo); try { - const res = await setupRequest(req, payload).expect(200); + const res = await setupRequest(req, payload).expect(isOK); } catch (err) { assert(false, err); } @@ -175,16 +175,33 @@ describe("the webhook server", function() { ghMock.pr("acme/repo", prNumber, testPreviewLink, "test.bs", test_sha, new Date("2000-01-01")); const payload = editPrPayload("pr-preview[bot]", "acme/repo"); try { - const res = await setupRequest(req, payload).expect(200); + const res = await setupRequest(req, payload).expect(isOK); + } catch (err) { + assert(false, err); + } + }); + + it("does not ignore PR edits from pr-preview bot on pull requests older than the crawled version of the spec, but within 15 minutes of that time", async () => { + const spec = getSpec("single-page"); + const repo = spec.nightly.repository.split("/").slice(3).join("/"); + ghMock.pr("acme/repo", prNumber, testPreviewLink, "test.bs", test_sha, new Date("Mon, 27 Mar 2023 14:40:14 GMT")); + ghMock.listComments(repo, prNumber, []); + ghMock.prComment(repo, prNumber, + formatReport(removedTargets(["https://example.com/single-page#valid1"]), spec)); + mockPreviewSpec(testPreviewLink); + const payload = editPrPayload("pr-preview[bot]", "acme/repo"); + try { + const res = await setupRequest(req, payload).expect(isOK); } catch (err) { assert(false, err); } }); + it("ignores other PR edits (not from pr-preview bot)", async () => { const payload = editPrPayload("dontcallmedom", "acme/repo"); try { - const res = await setupRequest(req, payload).expect(200); + const res = await setupRequest(req, payload).expect(isOK); } catch (err) { assert(false, err); } @@ -194,7 +211,7 @@ describe("the webhook server", function() { const payload = editPrPayload("pr-preview[bot]", "acme/repo"); payload.action = "created"; try { - const res = await setupRequest(req, payload).expect(200); + const res = await setupRequest(req, payload).expect(isOK); } catch (err) { assert(false, err); } @@ -203,7 +220,7 @@ describe("the webhook server", function() { it("ignores other github events", async () => { const payload = editPrPayload("pr-preview[bot]", "acme/repo"); try { - const res = await setupRequest(req, payload, "issue").expect(200); + const res = await setupRequest(req, payload, "issue").expect(isOK); } catch (err) { assert(false, err); } @@ -213,12 +230,12 @@ describe("the webhook server", function() { afterEach(() => { assert.deepEqual(ghMock.errors, []); agent.assertNoPendingInterceptors(); - assert.deepEqual(nock.pendingMocks(), [], "All nock-mocked requested have been triggered"); }); after(async () => { server.close(); - agent.close(); + await agent.close(); + setGlobalDispatcher(origDispatcher); }); }); diff --git a/test/github.js b/test/github.js index 6357941..4d35ffd 100644 --- a/test/github.js +++ b/test/github.js @@ -6,7 +6,7 @@ const { baseDirUrl } = require('../lib/util'); const Github = require('../lib/github'); const assert = require("assert"); -const { MockAgent, setGlobalDispatcher } = require('undici'); +const { MockAgent, setGlobalDispatcher, getGlobalDispatcher } = require('undici'); const GhMock = require("./gh-api-mock"); @@ -29,6 +29,7 @@ const testWhatwgMultiPreviewLinks = [`https://whatpr.org/reponame-multi/${prNumb const testWhatwgMultiPreviewIndex = `https://whatpr.org/reponame-multi/${prNumber}/index.html`; let github; +const origDispatcher = getGlobalDispatcher(); function setup() { agent = new MockAgent(); @@ -42,6 +43,7 @@ async function teardown() { assert.deepEqual(ghMock.errors, [], "No GH API mocking errors should have happened"); agent.assertNoPendingInterceptors(); await agent.close(); + setGlobalDispatcher(origDispatcher); } describe("The PR Parser", () => { @@ -78,7 +80,7 @@ describe("The PR Parser", () => { assert(false, "No error thrown when one was expected"); }); - after(teardown); + after(async () => await teardown()); }); describe("The Report Finder", () => { @@ -99,6 +101,6 @@ describe("The Report Finder", () => { ]); assert.deepEqual(await github.findReport(testRepo, prNumber, "removedtargets"), undefined); }); - after(teardown); + after(async() => await teardown()); });