diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3d347dc7..9d9bc4e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,3 +44,8 @@ npm run all # all of the above Running `npm run all` is a good way to confirm your change compiles and lints. It will modify `dist/` as a side effect. That is fine, just leave those changes out of your commits. + +To try your change in a real workflow, commit the built `dist/` to a separate +branch in your fork and point a workflow at it with +`uses: /set-commit-status-action@`. Open the pull +request from a branch without that commit. diff --git a/eslint.config.mjs b/eslint.config.mjs index 6dff86f0..301768b0 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -1,4 +1,5 @@ import MyrotvoretsConfig from '@myrotvorets/eslint-config-myrotvorets-ts'; +import globals from 'globals'; /** @type {import('eslint').Linter.Config[]} */ export default [ @@ -6,4 +7,14 @@ export default [ ignores: ['dist/**', 'lib/**'], }, ...MyrotvoretsConfig, + { + // Tests are plain CommonJS running under Node's built-in test runner, + // not part of the TypeScript build. no-undef stays on for them, unlike + // for .ts where tsc covers it, so the globals have to be declared. + files: ['tests/**/*.js'], + languageOptions: { + sourceType: 'commonjs', + globals: globals.node, + }, + }, ]; diff --git a/package-lock.json b/package-lock.json index 0a8f2588..a8928080 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,6 +15,7 @@ "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", + "globals": "^17.11.0", "typescript": "^5.3.3" }, "engines": { @@ -2895,7 +2896,6 @@ "integrity": "sha512-Z2I8hM+PbJDXQDq3Icgpzv+mPdwr68iZUU9d5WW4FuXfDUQfkZaZuvjMv42/5crNyw154+9+VWXbYrUgDXbxNw==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=18" }, diff --git a/package.json b/package.json index d5a7dc58..1de824a0 100644 --- a/package.json +++ b/package.json @@ -8,8 +8,9 @@ "lint": "eslint .", "lint:fix": "eslint . --fix", "package": "ncc build --minify", - "test": "exit 0", - "all": "npm run build && npm run lint:fix && npm run package && npm test" + "test": "npm run build && npm run test:unit", + "all": "npm run build && npm run lint:fix && npm run package && npm run test:unit", + "test:unit": "node --test \"tests/*.test.js\"" }, "keywords": [], "author": "Myrotvorets (https://myrotvorets.center/)", @@ -21,6 +22,7 @@ "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", + "globals": "^17.11.0", "typescript": "^5.3.3" }, "repository": { diff --git a/src/main.ts b/src/main.ts index 529e540b..6eb573d3 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,8 +1,8 @@ import { getInput, info, setFailed, warning } from '@actions/core'; import { getOctokit } from '@actions/github'; -import { RequestError } from '@octokit/request-error'; import { CommitStatusState, + getApiErrorMessage, getCommitHash, isForeignPullRequest, parseRepoName, @@ -21,10 +21,6 @@ interface Inputs { context?: string; } -interface ValidationFailedError { - errors: object | string; -} - function getInputs(): Inputs { const token = getInput('token', { required: true }); const state = validateCommitStatusState(getInput('status', { required: true })); @@ -71,18 +67,7 @@ async function run(): Promise { context: inputs.context, }); } catch (error) { - let foundMoreElaborateError = false; - const reqError = error as RequestError; - if (reqError.response !== undefined) { - const errorData = reqError.response.data as ValidationFailedError; - if (typeof errorData.errors === 'string') { - foundMoreElaborateError = true; - setFailed(errorData.errors); - } - } - if (!foundMoreElaborateError) { - setFailed((error as Error).message); - } + setFailed(getApiErrorMessage(error)); } } diff --git a/src/utils.ts b/src/utils.ts index 8af84684..f883866f 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -3,6 +3,48 @@ import type { PullRequest } from '@octokit/webhooks-types'; export type CommitStatusState = 'error' | 'failure' | 'pending' | 'success'; +/** + * The shape we read off a failed API call. Only the one field we actually use, + * described locally rather than imported from `@octokit/request-error`. + * + * Every level is optional because none of it is guaranteed: the throw may not be + * a request error at all, and `data` has been observed as `null`. Reading it + * structurally also avoids `instanceof RequestError`, which fails silently when + * more than one copy of that package ends up in the tree. + */ +interface RequestErrorLike { + message?: string; + response?: { + data?: { + errors?: unknown; + }; + }; +} + +/** + * Returns the most specific message available for a failed API call. + * + * The GitHub API sometimes puts a useful explanation in `response.data.errors` + * while `error.message` is only "Validation Failed". Prefer the former when it + * is a string, and fall back to the error's own message otherwise. + */ +export function getApiErrorMessage(error: unknown): string { + const errorLike = error as RequestErrorLike | null; + + if (typeof errorLike?.response?.data?.errors === 'string') { + return errorLike.response.data.errors; + } + + // Read `message` structurally rather than testing `instanceof Error`, for the + // same reason as above, and so an error-like object that carries a message + // still reports it instead of being stringified to "[object Object]". + if (typeof errorLike?.message === 'string') { + return errorLike.message; + } + + return String(error); +} + function isPullRequest(): boolean { return context.eventName === 'pull_request'; } diff --git a/tests/utils.test.js b/tests/utils.test.js new file mode 100644 index 00000000..65cc8f87 --- /dev/null +++ b/tests/utils.test.js @@ -0,0 +1,76 @@ +// Plain CommonJS against the compiled output in lib/, which is what `npm run +// build` produces and what ncc then bundles into dist/. Node's built-in test +// runner is used deliberately: this repo ships a bundle to other people's +// runners, so not adding a test framework is worth a little verbosity. + +const { strictEqual } = require('node:assert'); +const { describe, it } = require('node:test'); + +const { getApiErrorMessage } = require('../lib/utils.js'); + +describe('getApiErrorMessage', () => { + it('prefers response.data.errors when it is a string', () => { + const error = Object.assign(new Error('Validation Failed'), { + response: { data: { errors: 'Sha is not a valid commit' } }, + }); + + strictEqual(getApiErrorMessage(error), 'Sha is not a valid commit'); + }); + + it('falls back to the message when errors is an array', () => { + // The shape the API returns most of the time. Deliberately not flattened, + // so this documents that the elaborate path stays dormant for it. + const error = Object.assign(new Error('Validation Failed'), { + response: { data: { errors: [{ resource: 'Status', code: 'custom' }] } }, + }); + + strictEqual(getApiErrorMessage(error), 'Validation Failed'); + }); + + it('falls back to the message when there is no response', () => { + strictEqual(getApiErrorMessage(new Error('Bad credentials')), 'Bad credentials'); + }); + + it('does not throw when data is null', () => { + const error = Object.assign(new Error('Not Found'), { response: { data: null } }); + + strictEqual(getApiErrorMessage(error), 'Not Found'); + }); + + it('does not throw when the response has no data', () => { + const error = Object.assign(new Error('Server Error'), { response: {} }); + + strictEqual(getApiErrorMessage(error), 'Server Error'); + }); + + it('uses the message from an error-like object', () => { + strictEqual(getApiErrorMessage({ message: 'Bad credentials' }), 'Bad credentials'); + }); + + it('handles a thrown value that is not an Error', () => { + strictEqual(getApiErrorMessage('boom'), 'boom'); + }); + + it('handles a thrown null', () => { + strictEqual(getApiErrorMessage(null), 'null'); + }); + + it('handles a thrown undefined', () => { + strictEqual(getApiErrorMessage(undefined), 'undefined'); + }); + + it('does not throw when the response is null', () => { + const error = Object.assign(new Error('Not Found'), { response: null }); + + strictEqual(getApiErrorMessage(error), 'Not Found'); + }); + + it('falls back to the message when data is not an object', () => { + // e.g. a proxy returning an HTML error page instead of JSON. + for (const data of ['Bad Gateway', 502]) { + const error = Object.assign(new Error('Bad Gateway'), { response: { data } }); + + strictEqual(getApiErrorMessage(error), 'Bad Gateway'); + } + }); +}); diff --git a/tsconfig.json b/tsconfig.json index 8dacd5ff..7b70c001 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -2,6 +2,10 @@ "compilerOptions": { "target": "es2019", "module": "CommonJS", + // Pinned so the emit layout cannot shift. Without it tsc infers rootDir + // from the inputs, and any file added outside src/ would relocate the + // output to lib/src/, breaking the "main" entry that ncc bundles from. + "rootDir": "./src", "noEmitOnError": true, "outDir": "./lib", "sourceMap": false, @@ -18,5 +22,5 @@ ] }, "compileOnSave": true, - "include": ["src/**.ts", "tests/**.ts"] + "include": ["src/**.ts"] }