Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions __tests__/api/webhook/github.route.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
* @jest-environment node
*/
import crypto from "crypto"
import { NextRequest } from "next/server"

jest.mock(
"@/lib/webhook/github/handlers/installation/revalidateRepositoriesCache.handler",
() => ({
revalidateUserInstallationReposCache: jest.fn(),
})
)
jest.mock("@/lib/webhook/github/handlers/issue/label.resolve.handler", () => ({
handleIssueLabelResolve: jest.fn(),
}))
jest.mock("@/lib/webhook/github/handlers/pullRequest/closed.removeContainer.handler", () => ({
handlePullRequestClosedRemoveContainer: jest.fn(),
}))
jest.mock("@/lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler", () => ({
handlePullRequestLabelCreateDependentPR: jest.fn(),
}))
jest.mock("@/lib/webhook/github/handlers/repository/edited.revalidateRepoCache.handler", () => ({
handleRepositoryEditedRevalidate: jest.fn(),
}))
import { POST } from "@/app/api/webhook/github/route"
import { handleIssueLabelAutoResolve } from "@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler"

jest.mock("@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler", () => ({
handleIssueLabelAutoResolve: jest.fn(),
}))

describe("POST /api/webhook/github", () => {
const secret = "test-secret"
const originalSecret = process.env.GITHUB_WEBHOOK_SECRET

beforeEach(() => {
jest.resetAllMocks()
process.env.GITHUB_WEBHOOK_SECRET = secret
})

afterAll(() => {
process.env.GITHUB_WEBHOOK_SECRET = originalSecret
})

it("routes i2pr resolve issue label payloads to the auto-resolve handler", async () => {
const payload = {
action: "labeled",
label: { name: "i2pr: resolve issue" },
repository: { full_name: "octo-org/octo-repo" },
issue: { number: 42 },
sender: { login: "octocat" },
installation: { id: 9876 },
}

const rawBody = Buffer.from(JSON.stringify(payload))
const signature =
"sha256=" + crypto.createHmac("sha256", secret).update(rawBody).digest("hex")

const headers = new Headers({
"x-hub-signature-256": signature,
"x-github-event": "issues",
})

const mockRequest = {
headers,
arrayBuffer: jest.fn().mockResolvedValue(rawBody),
} as unknown as NextRequest

const response = await POST(mockRequest)

expect(response.status).toBe(200)

expect(handleIssueLabelAutoResolve).toHaveBeenCalledTimes(1)
const callArgs = jest.mocked(handleIssueLabelAutoResolve).mock.calls[0]?.[0]
expect(callArgs).toBeDefined()
expect(callArgs.installationId).toBe(String(payload.installation.id))
expect(callArgs.payload.repository.full_name).toBe(payload.repository.full_name)
expect(callArgs.payload.issue.number).toBe(payload.issue.number)
expect(callArgs.payload.sender.login).toBe(payload.sender.login)
})
})
3 changes: 3 additions & 0 deletions __tests__/config/jest.config.base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ const baseConfig: Config = {
"^@/__tests__/(.*)$": "<rootDir>/__tests__/$1",
"^@shared/(.*)$": "<rootDir>/shared/src/$1",
"^@workers/(.*)$": "<rootDir>/apps/workers/src/$1",

// NEW: allow direct `shared/...` imports in tests
"^shared/(.*)$": "<rootDir>/shared/src/$1",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, along with the setupEnv.test.ts, seems unrelated to this PR. I would wish we could carve out these changes in a separate PR from main, and merge that carved out PR first, before reviewing this PR.

},
coveragePathIgnorePatterns: ["/node_modules/", "/.next/", "/coverage/"],
rootDir: "../..",
Expand Down
61 changes: 40 additions & 21 deletions __tests__/lib/utils/setupEnv.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ jest.mock("node:child_process", () => {
}
})

import type {
ChildProcess,
ExecException,
ExecOptions,
} from "node:child_process"
import { exec as execCallback } from "node:child_process"

import { setupEnv } from "@/lib/utils/cli"
Expand All @@ -13,6 +18,13 @@ const execMock = execCallback as unknown as jest.MockedFunction<
typeof execCallback
>

// Typed callback used by our mock
type ExecCallback = (
error: ExecException | null,
stdout: string,
stderr: string
) => void

describe("setupEnv utility", () => {
const baseDir = process.cwd()

Expand All @@ -21,20 +33,21 @@ describe("setupEnv utility", () => {
})

it("returns confirmation when 'pnpm i' succeeds", async () => {
// Arrange mocked successful exec
execMock.mockImplementation(
(
cmd: string,
options: unknown,
callback: (error: Error | null, stdout: string, stderr: string) => void
) => {
// Support optional options argument
if (typeof options === "function") {
callback = options
}
optionsOrCallback?: ExecOptions | ExecCallback | null,
maybeCallback?: ExecCallback | null
): ChildProcess => {
const callback: ExecCallback =
typeof optionsOrCallback === "function"
? optionsOrCallback
: (maybeCallback as ExecCallback)

callback(null, "installation complete", "")
// Return dummy ChildProcess object
return {}

// We don't care about the ChildProcess in this test; satisfy TS with a cast.
return {} as ChildProcess
}
)

Expand All @@ -50,21 +63,27 @@ describe("setupEnv utility", () => {
})

it("throws a helpful error when the command fails", async () => {
// Arrange mocked failing exec
execMock.mockImplementation(
(
cmd: string,
options: unknown,
callback: (error: Error | null, stdout: string, stderr: string) => void
) => {
if (typeof options === "function") {
callback = options
}
const error = new Error("mock failure")
error.stdout = "some stdout"
error.stderr = "some stderr"
optionsOrCallback?: ExecOptions | ExecCallback | null,
maybeCallback?: ExecCallback | null
): ChildProcess => {
const callback: ExecCallback =
typeof optionsOrCallback === "function"
? optionsOrCallback
: (maybeCallback as ExecCallback)

const error: ExecException & {
stdout: string
stderr: string
} = Object.assign(new Error("mock failure"), {
stdout: "some stdout",
stderr: "some stderr",
})

callback(error, "some stdout", "some stderr")
return {}
return {} as ChildProcess
}
)

Expand Down
27 changes: 26 additions & 1 deletion app/api/webhook/github/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { revalidateUserInstallationReposCache } from "@/lib/webhook/github/handl
import { handleIssueLabelAutoResolve } from "@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler"
import { handleIssueLabelResolve } from "@/lib/webhook/github/handlers/issue/label.resolve.handler"
import { handlePullRequestClosedRemoveContainer } from "@/lib/webhook/github/handlers/pullRequest/closed.removeContainer.handler"
import { handlePullRequestLabelCreateDependentPR } from "@/lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler"
import { handleRepositoryEditedRevalidate } from "@/lib/webhook/github/handlers/repository/edited.revalidateRepoCache.handler"
import {
CreatePayloadSchema,
Expand Down Expand Up @@ -167,8 +168,31 @@ export async function POST(req: NextRequest) {
}
break
}
case "labeled": {
const installationId = String(parsedPayload.installation?.id ?? "")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think based on the PullRequestPayloadSchema, the installation ID should exist (it is NOT optional). So if it didn't exist in the payload, then our schema.safeParse will already catch that error.

If the rest of our subscribers to the PullRequestPayloadSchema is also converting it to a String, then might as well just make the whole schema installataionId into a string, instead of number.

if (!installationId) {
console.error(
"[ERROR] No installation ID found in webhook payload"
)
return new Response("No installation ID found", { status: 400 })
}
const labelName: string | undefined =
parsedPayload.label?.name?.trim()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole structure of this file feels a bit jumbled and it's hard to read.

Because we're mixing switch statements, and in between each switch statement, we're doing additional parsing.

It would be nice if we could move all the parsing to 1 section, including the converting and trimming and extracting data, maybe to the top of the file somehow, before we start entering the switch statements.

That way we can just collapse the nested switch statements into a more cohesive tree, without having to see all the parsing functionality interspersed within the tree.

switch (labelName?.toLowerCase()) {
case "i2pr: update pr": {
await handlePullRequestLabelCreateDependentPR({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we could have a few different tests built out in the /tests folder somehow.

I'm not sure how to best structure tests, but my initial thinking is we'd need:

  • a test to see if a webhook event gets captured by our API here. So if we could send a mock webhook event, and see if our webhook catches it, including this new implementation for the 'labeled' action.
  • A test to see if our webhook can correctly create a job for our job queue. I'm guessing there would also be a mock for queueing job events.
  • A test to see if our worker would pick up the job correctly and launch the workflow. I guess the workflow itself could also have a mock (so we don't actually launch any workflows).

How are tests often structured? I'm still new to testing.

payload: parsedPayload,
installationId,
})
break
}
default:
// Unhandled label; ignore
break
}
break
}
case "opened":
case "labeled":
case "synchronize":
case "reopened":
// Explicitly supported as no-ops for now
Expand Down Expand Up @@ -340,3 +364,4 @@ export async function POST(req: NextRequest) {
return new Response("Error", { status: 500 })
}
}

16 changes: 15 additions & 1 deletion apps/workers/workflow-workers/src/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export async function handler(job: Job): Promise<string> {
if (!job.id) {
await publishJobStatus("unknown", "Failed: Job ID is required")
throw new Error("Job ID is required")
}
}

await publishJobStatus(job.id, "Parsing job")

Expand Down Expand Up @@ -66,6 +66,19 @@ export async function handler(job: Job): Promise<string> {
)
return result.map((m) => m.content).join("\n")
}
case "createDependentPR": {
// Stub implementation for now; the full workflow runs in the web app.
await publishJobStatus(
job.id,
"Job: Create dependent PR (dispatching to web workflow runner)"
)
// In a future iteration, this worker can invoke a shared usecase.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we need to actually implement this.

I think previously for other workflows, like autoResolveIssue, we've had to migrate their implementations from the /lib folder into the /shared folder somehow.

Once, I just copied the entire implementation into the /shared folder, with similar folder structure, and vowed to refactor in a future date. We can do something similar here, if needed.

The worker must be able to run the createDependentPR job.

await publishJobStatus(
job.id,
"Completed: createDependentPR job dispatched"
)
return "createDependentPR dispatched"
}
default: {
await publishJobStatus(job.id, "Failed: Unknown job name")
throw new Error(`Unknown job name: ${job.name}`)
Expand All @@ -77,3 +90,4 @@ export async function handler(job: Job): Promise<string> {
throw error instanceof Error ? error : new Error(msg)
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { QueueEnum, WORKFLOW_JOBS_QUEUE } from "shared/entities/Queue"
import { addJob } from "shared/services/job"

import type { PullRequestPayload } from "@/lib/webhook/github/types"

/**
* Handler: PR labeled with "I2PR: Update PR"
* - Enqueues the createDependentPR job onto the workflow-jobs queue
* - Includes installation id and labeler login in job data
*/
export async function handlePullRequestLabelCreateDependentPR({
payload,
installationId,
}: {
payload: PullRequestPayload
installationId: string
}) {
const redisUrl = process.env.REDIS_URL
if (!redisUrl) {
throw new Error("REDIS_URL is not set")
}

const owner = payload.repository?.owner?.login
const repo = payload.repository?.name
const pullNumber = payload.number || payload.pull_request?.number
const githubLogin = payload.sender?.login

if (!owner || !repo || typeof pullNumber !== "number" || !githubLogin) {
throw new Error(
"Missing required fields for createDependentPR job (owner, repo, pullNumber, sender.login)"
)
}

const repoFullName = `${owner}/${repo}`
const queue: QueueEnum = WORKFLOW_JOBS_QUEUE

await addJob(
queue,
{
name: "createDependentPR",
data: {
repoFullName,
pullNumber,
githubLogin,
githubInstallationId: installationId,
},
},
{},
redisUrl
)
}

5 changes: 5 additions & 0 deletions lib/webhook/github/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@ export type IssuesPayload = z.infer<typeof IssuesPayloadSchema>

export const PullRequestPayloadSchema = z.object({
action: z.string(),
number: z.number().optional(),
label: z.object({ name: z.string() }).optional(),
sender: z.object({ login: z.string() }).optional(),
pull_request: z.object({
merged: z.boolean().optional(),
head: z.object({ ref: z.string() }).optional(),
number: z.number().optional(),
}),
repository: z.object({
name: z.string(),
Expand Down Expand Up @@ -160,3 +164,4 @@ export const RepositoryPayloadSchema = z.discriminatedUnion("action", [
])

export type RepositoryPayload = z.infer<typeof RepositoryPayloadSchema>

16 changes: 16 additions & 0 deletions shared/src/entities/events/Job.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,26 @@ export const AutoResolveIssueJobSchema = z.object({

export type AutoResolveIssueJob = z.infer<typeof AutoResolveIssueJobSchema>

export const CreateDependentPRJobSchema = z.object({
name: z.literal("createDependentPR"),
data: z.object({
repoFullName: z.string(),
pullNumber: z.number().int().positive(),
githubLogin: z.string(),
githubInstallationId: z.string(),
}),
})

export type CreateDependentPRJob = z.infer<
typeof CreateDependentPRJobSchema
>

export const JobEventSchema = z.discriminatedUnion("name", [
SummarizeIssueJobSchema,
SimulateLongRunningWorkflowJobSchema,
AutoResolveIssueJobSchema,
CreateDependentPRJobSchema,
])

export type JobEvent = z.infer<typeof JobEventSchema>