Skip to content

Commit

Permalink
Merge pull request #12 from dontcallmedom/looser-time-check
Browse files Browse the repository at this point in the history 
Take into account spec generation time when comparing PR and spec date
  • Loading branch information
@dontcallmedom
dontcallmedom authored Oct 16, 2023
2 parents c212e3f + eb642f1 commit 12fd5f0
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 40 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/test.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down
41 changes: 25 additions & 16 deletions gh-webhook.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -35,31 +39,36 @@ 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));
console.trace(err);
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;
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
{
"dependencies": {
"@js-temporal/polyfill": "^0.4.4",
"@octokit/rest": "^19.0.7",
"@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"
},
Expand Down
51 changes: 34 additions & 17 deletions test/gh-webhook.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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]);
}
}
Expand Down Expand Up @@ -109,6 +108,7 @@ const removedTargets = (links) => {
}];
};

const isOK = res => res.status === 200 || res.status === 202;

describe("the webhook server", function() {
this.timeout(20000);
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
});

});
Expand Down
8 changes: 5 additions & 3 deletions test/github.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand All @@ -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();
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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", () => {
Expand All @@ -99,6 +101,6 @@ describe("The Report Finder", () => {
]);
assert.deepEqual(await github.findReport(testRepo, prNumber, "removedtargets"), undefined);
});
after(teardown);
after(async() => await teardown());
});

0 comments on commit 12fd5f0

Please sign in to comment.