From 6717873570ef7b085adec9d6582e7a5c46cc37be Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 4 Dec 2024 13:10:30 +0100 Subject: [PATCH] Make commit information opt-in (#107) * Make commit information optional * Adapt tests --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- .changeset/long-forks-dream.md | 49 ++++++++++++ config/knip.ts | 1 + docs/review/api/alfa-test-utils.api.md | 23 +++--- .../docs/usage/reporting/basic.md | 2 +- .../docs/usage/reporting/configuration.md | 39 +++++++-- packages/alfa-test-utils/package.json | 2 + .../src/report/commit-information.ts | 41 ++++++++++ packages/alfa-test-utils/src/report/git.ts | 39 +-------- packages/alfa-test-utils/src/report/index.ts | 15 +++- packages/alfa-test-utils/src/report/sip.ts | 24 +++--- packages/alfa-test-utils/src/tsconfig.json | 7 +- .../alfa-test-utils/test/report/git.spec.ts | 4 +- .../alfa-test-utils/test/report/sip.spec.tsx | 79 ++++++------------- 13 files changed, 190 insertions(+), 135 deletions(-) create mode 100644 .changeset/long-forks-dream.md create mode 100644 packages/alfa-test-utils/src/report/commit-information.ts diff --git a/.changeset/long-forks-dream.md b/.changeset/long-forks-dream.md new file mode 100644 index 00000000..aef6b723 --- /dev/null +++ b/.changeset/long-forks-dream.md @@ -0,0 +1,49 @@ +--- +"@siteimprove/alfa-test-utils": minor +--- + +**Breaking:** `CommitInformation` is now optional and some of its properties have been rennamed; see the package's changelog for explanations and migration advice. + +Previously, `SIP.upload` was automatically collecting some information about the latest `git` commit and sending it to the Siteimprove Intelligence Platform, unless opted out via the `includeGitInfo: false` option. This presented two main drawbacks: + +1. (minor) This was heavily reliant on the directory being part of a `git` repository. For codebases that use a different version control system, not only this was useless, but no alternative was provided. +2. (major) This was heavily reliant on the Accessibility Code Checker running from a NodeJS environment, where access to the underlying filesystem and `git` was doable. Not only this prevented the Accessibility Code Checker to run seamlessly from other environments such as browser extensions or Cypress; but the mere fact of trying to bundle `SIP.upload` for such environments (e.g. with Webpack) was causing the build to fail. + +As a consequence, this release reverts a bit the approach on that information. The commit information is still valuable and can be used to name or group tests in an organised way (e.g., to follow the number of issues in a feature branch), but it now has to be provided by the caller. A `git` helper is still provided since it is by far the most used version control system. + +Concretely, the following changes have been made: + +1. The `CommitInformation.GitOrigin` field has been renamed `CommitInformation.Origin`. +2. `SIP.upload` now accepts a `commitInformation` option, of type `CommitInformation`; this in an object that must at least contain a `BranchName` field with a string value. (note the starting uppercase in `BranchName`). +3. If no `commitInformation` is provided, but the `testName` is a function, it will resolve to the default "Unnamed" test. +4. The `includeGitInfo` options of `SIP.upload` has been removed; if a `CommitInformation` has been provided, it will automatically be uploaded. + +In order to migrate from previous versions: + +1. If you were using `includeGitInfo: false` to opt-out of it, simply remove the option as it is now an opt-in information. +2. If you were using a `testName` function; you need to provide the commit information to the call. For this, use: + + ````typescript + import { getCommitInformation } from "@siteimprove/alfa-test-utils/git.js"; + + const gitInformation = await getCommitInformation(); + + SIP.upload({ + ... // other options + commitInformation: gitInformation, + testName: (commitInfo) => ... // same function as previously + }) + ``` +3. If you were not previously relying on the commit information but wish to keep uploading it, use: + ````typescript + import { getCommitInformation } from "@siteimprove/alfa-test-utils/git.js"; + + const gitInformation = await getCommitInformation(); + + SIP.upload({ + ... // other options + commitInformation: gitInformation, + }) + ``` + +Siteimprove recommends that you provide some basic commit information as it opens possibilities for filtering of the results and better reporting in the Siteimprove Intelligence Platform. As of November 2024, we are currently not relying on this information in any place, but it is likely that we will provide features using it in the future; therefore it might be easier to start providing it immediately instead of revisiting the codebase later. diff --git a/config/knip.ts b/config/knip.ts index 6852444c..ea9315d5 100644 --- a/config/knip.ts +++ b/config/knip.ts @@ -15,6 +15,7 @@ const config: KnipConfig = { ], }, "packages/alfa-cli": { entry: ["src/alfa.ts!"], project }, + "packages/alfa-test-utils": { entry: [...entry, "src/report/git.ts!"], project }, "packages/alfa-webdriver": { entry, project, diff --git a/docs/review/api/alfa-test-utils.api.md b/docs/review/api/alfa-test-utils.api.md index 46800c0e..1ba2dc91 100644 --- a/docs/review/api/alfa-test-utils.api.md +++ b/docs/review/api/alfa-test-utils.api.md @@ -88,22 +88,17 @@ export namespace Audit { export function run(page: Page, options?: Options): Promise; } -// @public (undocumented) +// @public export interface CommitInformation { - Author: string | undefined; + Author?: string; BranchName: string; - CommitHash: string | undefined; - CommitTimestamp: string | undefined; - Email: string | undefined; - GitOrigin: string | undefined; - Message: string | undefined; + CommitHash?: string; + CommitTimestamp?: string; + Email?: string; + Message?: string; + Origin?: string; } -// Warning: (ae-internal-missing-underscore) The name "getCommitInformation" should be prefixed with an underscore because the declaration is marked as @internal -// -// @internal (undocumented) -export function getCommitInformation(): Promise>; - // @public export class Logging implements Equatable, json.Serializable { // (undocumented) @@ -272,11 +267,11 @@ export namespace SIP { // (undocumented) export interface Options { apiKey?: string; - includeGitInfo?: boolean; + commitInformation?: CommitInformation; pageTitle?: string | ((page: Page) => string); pageURL?: string | ((page: Page) => string); siteID?: string; - testName?: string | ((git: CommitInformation) => string); + testName?: string | ((commit: CommitInformation) => string); userName?: string; } // @internal diff --git a/packages/alfa-test-utils/docs/usage/reporting/basic.md b/packages/alfa-test-utils/docs/usage/reporting/basic.md index 1cf8c536..f791c6fb 100644 --- a/packages/alfa-test-utils/docs/usage/reporting/basic.md +++ b/packages/alfa-test-utils/docs/usage/reporting/basic.md @@ -21,7 +21,7 @@ const pageReportURL = await SIP.upload(alfaResult, { This returns a URL to a Siteimprove Intelligence Platform page report showing the audit results, embedded within a [`Result` object](https://github.com/Siteimprove/alfa/blob/main/docs/api/alfa-result.md) to handle errors, use `console.log(pageReportURL.getOrElse(() => pageReportURL.getErrUnsafe()));` to show its value. -> **Note:** By default, when ran from a `git` repository, this will upload some [basic git information](https://github.com/Siteimprove/alfa-integrations/blob/main/docs/api/alfa-test-utils.commitinformation.md) if you prefer not to do so, add the `includeGitInfo: false` option to the second argument of `SIP.upload`. +> **Note:** Siteimprove recommends that you include some basic information about the latest commit together with the upload, at least the branch name. This opens possibilities of grouping and reporting based on it, e.g. to follow the number of issues in a given branch. See [common configuration](./configuration.md#including-commit-information) for more information. ## Pretty printing results in the console diff --git a/packages/alfa-test-utils/docs/usage/reporting/configuration.md b/packages/alfa-test-utils/docs/usage/reporting/configuration.md index 0b1173d3..1d0ce710 100644 --- a/packages/alfa-test-utils/docs/usage/reporting/configuration.md +++ b/packages/alfa-test-utils/docs/usage/reporting/configuration.md @@ -52,14 +52,41 @@ const pageReportURL = Audit.run(alfaPage, { }); ``` -## Skipping git information +## Including commit information -If you prefer not to upload basic git information about the repository (origin URL; branch name; latest commit hash, author, and message), use the `includeGitInfo: false` option: +Siteimprove recommends that you include some basic information about the latest commit together with the upload, at least the branch name. This opens possibilities of grouping and reporting based on it, e.g. to follow the number of issues in a given branch. ```typescript -SIP.upload(alfaResult, { - userName: process.env.SI_USER_NAME!, - apiKey: process.env.SI_API_KEY!, - includeGitInfo: false, +const pageReportURL = Audit.run(alfaPage, { + rules: { include: Rules.aaFilter }, +}).then((alfaResult) => { + SIP.upload(alfaResult, { + userName: process.env.SI_USER_NAME!, + apiKey: process.env.SI_API_KEY!, + commitInformation: { + BranchName: "main", + CommitHash: "a1b2c3d4", + }, + }); +}); +``` + +See [the `CommitInforation` API](https://github.com/Siteimprove/alfa-integrations/blob/main/docs/api/alfa-test-utils.commitinformation.md) for all the allowed properties, only `BranchName` is mandatory. + +If running from a `git` repository (in a NodeJS environment), this commit information can be extracted automatically: + +```typescript +import { getCommitInformation } from "@siteimprove/alfa-test-utils/git.js"; + +const gitInformation = await getCommitInformation(); + +const pageReportURL = Audit.run(alfaPage, { + rules: { include: Rules.aaFilter }, +}).then((alfaResult) => { + SIP.upload(alfaResult, { + userName: process.env.SI_USER_NAME!, + apiKey: process.env.SI_API_KEY!, + commitInformation: gitInformation, + }); }); ``` diff --git a/packages/alfa-test-utils/package.json b/packages/alfa-test-utils/package.json index 8e3dab54..6b4eb589 100644 --- a/packages/alfa-test-utils/package.json +++ b/packages/alfa-test-utils/package.json @@ -21,6 +21,8 @@ ".": "./dist/index.js", "./audit": "./dist/audit/index.js", "./audit.js": "./dist/audit/index.js", + "./git": "./dist/report/git.js", + "./git.js": "./dist/report/git.js", "./report": "./dist/report/index.js", "./report.js": "./dist/report/index.js" }, diff --git a/packages/alfa-test-utils/src/report/commit-information.ts b/packages/alfa-test-utils/src/report/commit-information.ts new file mode 100644 index 00000000..20791b54 --- /dev/null +++ b/packages/alfa-test-utils/src/report/commit-information.ts @@ -0,0 +1,41 @@ +/** + * Global structure of a version control metadata. + * + * @remarks + * This is mostly modelled after git, but should work fine with other VCS like + * svn or cvs. Notably, only the branch name is mandatory. + * + * @public + */ +export interface CommitInformation { + /** + * The origin's URL. This may vary depending on whether the repository was cloned + * using `http` or `ssh` protocol. + */ + Origin?: string; + /** + * The name of the current branch. + */ + BranchName: string; + /** + * The hash of the latest commit. + */ + CommitHash?: string; + /** + * The name of the author of the latest commit. + */ + Author?: string; + /** + * The email of the author of the latest commit. + */ + Email?: string; + /** + * The timestamp of the latest commit. + */ + CommitTimestamp?: string; + /** + * The message of the latest commit. + */ + Message?: string; +} + diff --git a/packages/alfa-test-utils/src/report/git.ts b/packages/alfa-test-utils/src/report/git.ts index 64c92414..df9a9a5d 100644 --- a/packages/alfa-test-utils/src/report/git.ts +++ b/packages/alfa-test-utils/src/report/git.ts @@ -2,42 +2,9 @@ import { Err, Ok } from "@siteimprove/alfa-result"; import type { Result } from "@siteimprove/alfa-result"; import { simpleGit } from "simple-git"; -const git = simpleGit(); +import type { CommitInformation } from "./commit-information.js"; -/** - * @public - */ -export interface CommitInformation { - /** - * The origin's URL. This may vary depending on whether the repository was cloned - * using `http` or `ssh` protocol. - */ - GitOrigin: string | undefined; - /** - * The name of the current branch. - */ - BranchName: string; - /** - * The hash of the latest commit. - */ - CommitHash: string | undefined; - /** - * The name of the author of the latest commit. - */ - Author: string | undefined; - /** - * The email of the author of the latest commit. - */ - Email: string | undefined; - /** - * The timestamp of the latest commit. - */ - CommitTimestamp: string | undefined; - /** - * The message of the latest commit. - */ - Message: string | undefined; -} +const git = simpleGit(); /** @internal */ export async function getCommitInformation(): Promise< @@ -51,7 +18,7 @@ export async function getCommitInformation(): Promise< const latest = await git.log({ "--max-count": 1 }); const value: CommitInformation = { - GitOrigin: origin?.refs?.fetch, + Origin: origin?.refs?.fetch, BranchName: branch.current, CommitHash: latest?.latest?.hash, Author: latest?.latest?.author_name, diff --git a/packages/alfa-test-utils/src/report/index.ts b/packages/alfa-test-utils/src/report/index.ts index bbd5cb43..a6469c55 100644 --- a/packages/alfa-test-utils/src/report/index.ts +++ b/packages/alfa-test-utils/src/report/index.ts @@ -1,3 +1,16 @@ -export * from "./git.js"; +// git.js **MUST NOT** be re-exported automatically +// It depends on simple-git, that runs in Node and uses node-specific functions +// imported through the `node:` protocol. +// Due to this: +// 1. It cannot properly run in other environments, like the browser; thus +// making it useless for browser extensions, and hard to use in Cypress. +// 2. The mere fact of trying to bundle it for such environments (with Webpack or +// the likes) may crash the bundler. +// Therefore, we do not export it as part of the main package. It is still +// provided as a separate helper with its own `exports` path and has to be +// explicitly included in projects that need it. +// export * from "./git.js"; + +export * from "./commit-information.js" export * from "./logging.js"; export * from "./sip.js"; diff --git a/packages/alfa-test-utils/src/report/sip.ts b/packages/alfa-test-utils/src/report/sip.ts index 2209e7ef..d4416895 100644 --- a/packages/alfa-test-utils/src/report/sip.ts +++ b/packages/alfa-test-utils/src/report/sip.ts @@ -1,6 +1,7 @@ import { Array } from "@siteimprove/alfa-array"; import { Element, Query } from "@siteimprove/alfa-dom"; import { Map } from "@siteimprove/alfa-map"; +import { Option } from "@siteimprove/alfa-option"; import { Err, Ok } from "@siteimprove/alfa-result"; import type { Result } from "@siteimprove/alfa-result"; import type { Thunk } from "@siteimprove/alfa-thunk"; @@ -11,7 +12,7 @@ import axios from "axios"; import type { Agent as HttpsAgent } from "https"; import { Audit, type Performance } from "../audit/index.js"; -import { type CommitInformation, getCommitInformation } from "./git.js"; +import type { CommitInformation } from "./commit-information.js"; /** * Interacting with Siteimprove Intelligence Platform (SIP) API. @@ -118,19 +119,14 @@ export namespace SIP { /** * A name for the test (e.g. "AA conformance", …), or a function building a - * test name from the git commit information (e.g. the git hash or branch name). + * test name from the commit information (e.g. the commit hash, or the branch name). */ - testName?: string | ((git: CommitInformation) => string); + testName?: string | ((commit: CommitInformation) => string); /** - * Whether to upload git commit information to the Siteimprove Intelligence Platform - * (default: yes). - * - * @remarks - * If the directory is not in a git repository, or git is not installed, - * this will silently fail and not send any information. + * Information about the latest commit of a Version Control System. */ - includeGitInfo?: boolean; + commitInformation?: CommitInformation; } /** @@ -243,7 +239,7 @@ export namespace SIP { ? title(page()) : title; - const gitInfo = await getCommitInformation(); + const commitInfo = Option.from(options.commitInformation); const name = options.testName ?? Defaults.Name; const TestName = @@ -252,7 +248,7 @@ export namespace SIP { typeof name === "string" ? name : name !== undefined - ? gitInfo.map(name).getOrElse(() => gitInfo.getErrOr(Defaults.Name)) + ? commitInfo.map(name).getOr(Defaults.Name) : Defaults.Name; const result: Payload = { @@ -272,9 +268,7 @@ export namespace SIP { Durations: toCamelCase(audit.durations.common), }; - if ((options.includeGitInfo ?? true) && gitInfo.isOk()) { - result.CommitInformation = gitInfo.get(); - } + commitInfo.forEach((info) => (result.CommitInformation = info)); if (options.siteID !== undefined) { result.SiteId = options.siteID; diff --git a/packages/alfa-test-utils/src/tsconfig.json b/packages/alfa-test-utils/src/tsconfig.json index 55f18f78..d8acd3a1 100644 --- a/packages/alfa-test-utils/src/tsconfig.json +++ b/packages/alfa-test-utils/src/tsconfig.json @@ -10,10 +10,11 @@ "audit/outcomes.ts", "audit/performance.ts", "audit/rules.ts", - "report/logging.ts", + "report/commit-information.ts", + "report/get-rule-title.ts", "report/git.ts", "report/index.ts", - "report/sip.ts", - "report/get-rule-title.ts" + "report/logging.ts", + "report/sip.ts" ] } diff --git a/packages/alfa-test-utils/test/report/git.spec.ts b/packages/alfa-test-utils/test/report/git.spec.ts index bbfdc9fe..87f1145a 100644 --- a/packages/alfa-test-utils/test/report/git.spec.ts +++ b/packages/alfa-test-utils/test/report/git.spec.ts @@ -16,8 +16,8 @@ test("getCommitInformation reads info from git", async (t) => { const values = actual.getUnsafe(); - t.notEqual(values.GitOrigin, undefined); - t(values.GitOrigin!.includes("Siteimprove/alfa-integrations")); + t.notEqual(values.Origin, undefined); + t(values.Origin!.includes("Siteimprove/alfa-integrations")); for (const property of [ "BranchName", diff --git a/packages/alfa-test-utils/test/report/sip.spec.tsx b/packages/alfa-test-utils/test/report/sip.spec.tsx index 56669a46..4e2b186a 100644 --- a/packages/alfa-test-utils/test/report/sip.spec.tsx +++ b/packages/alfa-test-utils/test/report/sip.spec.tsx @@ -25,17 +25,8 @@ const { Verbosity } = Serializable; const { Metadata, S3 } = SIP; -/** - * Commit information is highly unstable, hence we simply test that it exist - * and delete it before deep equality testing. We could also use getCommitInformation - * again in the expected value, but then we would just test that it produces the - * same result twice… - */ - test("Metadata.payload() creates a payload", async (t) => { const actual = await Metadata.payload(makeAudit(), {}, timestamp); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload()); }); @@ -168,8 +159,6 @@ test("Metadata.payload() uses site ID if provided", async (t) => { { siteID: "12345" }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ SiteId: "12345" })); }); @@ -180,34 +169,41 @@ test("Metadata.payload() uses test name if provided", async (t) => { { testName: "test name" }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ TestName: "test name" })); }); -/** - * This test will fail if the origin is changed, e.g. on forks. - */ -test("Metadata.payload() builds test name from git information", async (t) => { +test("Metadata.payload() includes commit information if provided", async (t) => { + const actual = await Metadata.payload( + makeAudit(), + { commitInformation: { BranchName: "hello", Origin: "somewhere" } }, + timestamp + ); + + t.deepEqual( + actual, + makePayload({ + CommitInformation: { BranchName: "hello", Origin: "somewhere" }, + }) + ); +}); + +test("Metadata.payload() builds test name from commit information", async (t) => { const actual = await Metadata.payload( makeAudit(), - // Only the repo name is stable { - testName: (git) => - git.GitOrigin!.replace( - /.*Siteimprove\/alfa-integrations.*/, - "Siteimprove/alfa-integrations" - ), + commitInformation: { BranchName: "hello", Origin: "somewhere" }, + testName: (commit) => `On branch ${commit.BranchName}`, }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual( actual, - makePayload({ TestName: "Siteimprove/alfa-integrations" }) + makePayload({ + CommitInformation: { BranchName: "hello", Origin: "somewhere" }, + TestName: "On branch hello", + }) ); }); @@ -217,8 +213,6 @@ test("Metadata.payload() uses explicit title if provided", async (t) => { { pageTitle: "page title" }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ PageTitle: "page title" })); }); @@ -227,8 +221,6 @@ test("Metadata.payload() uses page's title if it exists", async (t) => { const page = makePage(h.document([Hello, ])); const actual = await Metadata.payload(makeAudit({ page }), {}, timestamp); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ PageTitle: "Hello" })); }); @@ -241,8 +233,6 @@ test("Metadata.payload() uses explicit title over page's title", async (t) => { { pageTitle: "page title" }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ PageTitle: "page title" })); }); @@ -255,8 +245,6 @@ test("Metadata.payload() builds page title from the page if specified", async (t { pageTitle: (page) => page.document.toString() }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual( actual, @@ -270,8 +258,6 @@ test("Metadata.payload() uses explicit URL if provided", async (t) => { { pageURL: "page URL" }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ PageUrl: "page URL" })); }); @@ -283,8 +269,6 @@ test("Metadata.payload() uses page's response's URL if it exists", async (t) => ); const actual = await Metadata.payload(makeAudit({ page }), {}, timestamp); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ PageUrl: "https://siteimprove.com/" })); }); @@ -300,8 +284,6 @@ test("Metadata.payload() uses explicit URL over page's URL", async (t) => { { pageURL: "page URL" }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ PageUrl: "page URL" })); }); @@ -314,23 +296,10 @@ test("Metadata.payload() builds page URL from the page if specified", async (t) { pageURL: (page) => page.response.status.toString() }, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual(actual, makePayload({ PageUrl: "200" })); }); -test("Metadata.payload() excludes commit information if requested", async (t) => { - const actual = await Metadata.payload( - makeAudit(), - { includeGitInfo: false }, - timestamp - ); - t.equal(actual.CommitInformation, undefined); - - t.deepEqual(actual, makePayload()); -}); - test("Metadata.payload() includes global durations", async (t) => { const actual = await Metadata.payload( makeAudit({ @@ -342,8 +311,6 @@ test("Metadata.payload() includes global durations", async (t) => { {}, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual( actual, @@ -369,8 +336,6 @@ test("Metadata.payload() includes rule durations in aggregates", async (t) => { {}, timestamp ); - t.notEqual(actual.CommitInformation, undefined); - delete actual.CommitInformation; t.deepEqual( actual,