Skip to content

Improve TestEnv for cleaner tests #4005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mbg merged 14 commits into from
Jul 11, 2026
+289 −148
Merged

Improve TestEnv for cleaner tests #4005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
31842a7
Simplify return types for `passes`
mbg Jul 9, 2026
b571644
Clean-up `passes` by splitting it into `passes` and `throws`
mbg Jul 9, 2026
a8ac1e0
Rename `TestEnv` to `EnvBuilder`
mbg Jul 9, 2026
a4319e3
Split `EnvBuilder` into it + `CallableEnvBuilder`
mbg Jul 9, 2026
6a78290
Add abstract `BaseEnvBuilder` to enforce that `clone` is overriden by…
mbg Jul 9, 2026
1a57d7a
Simplify checking logs
mbg Jul 9, 2026
c77051d
Allow `withActions` and `withEnv` to mutate values
mbg Jul 9, 2026
ab6fd52
Accept `Env` for `setupActionsVars`
mbg Jul 9, 2026
da75ca5
Add action vars helpers to `BaseEnvBuilder`
mbg Jul 9, 2026
6b3ba17
Fix typo
mbg Jul 9, 2026
a1e3f70
Add `satisfies` for state in `BaseEnvBuilder` constructor
mbg Jul 9, 2026
d0a69d7
Clone `env` and `actions` so that sinon stubs survive
mbg Jul 9, 2026
d8866c2
fixup! Add action vars helpers to `BaseEnvBuilder`
mbg Jul 9, 2026
9a9ab4c
Make `throwsAsync` arg `async`
mbg Jul 9, 2026
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
77 changes: 25 additions & 52 deletions src/config/file.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ test("getConfigFileInput returns undefined by default", async (t) => {
await callee(getConfigFileInput)
.withArgs({})
.withFeatures([Feature.ConfigFileRepositoryProperty])
.passes(async (fn) => t.is(await fn(), undefined));
.passes(t.is, undefined);
});

const repositoryProperties = {
Expand All @@ -22,75 +22,48 @@ const repositoryProperties = {

test("getConfigFileInput returns input value", async (t) => {
const testInput = "/some/path";
const target = callee(getConfigFileInput).withFeatures([
Feature.ConfigFileRepositoryProperty,
]);

const actionsEnv = target.getState().actions;
sinon
.stub(actionsEnv, "getOptionalInput")
.withArgs("config-file")
.returns(testInput);

// Even though both an input and repository property are configured,
// we prefer the direct input to the Action.
const targetWithArgs = target
.withActions(actionsEnv)
.withArgs(repositoryProperties);
await targetWithArgs.passes(async (fn) => t.is(await fn(), testInput));

// Check for the expected log message.
t.true(
targetWithArgs
.getLogger()
.hasMessage("Using configuration file input from workflow"),
);
await callee(getConfigFileInput)
.withFeatures([Feature.ConfigFileRepositoryProperty])
.withActions((actionsEnv) => {
sinon
.stub(actionsEnv, "getOptionalInput")
.withArgs("config-file")
.returns(testInput);
})
.withArgs(repositoryProperties)
.logs(t, "Using configuration file input from workflow")
.passes(t.is, testInput);
});

test("getConfigFileInput returns repository property value", async (t) => {
// Since there is no direct input, we should use the repository property.
const target = callee(getConfigFileInput)
await callee(getConfigFileInput)
.withFeatures([Feature.ConfigFileRepositoryProperty])
.withArgs(repositoryProperties);

await target.passes(async (fn) =>
t.is(await fn(), repositoryProperties[RepositoryPropertyName.CONFIG_FILE]),
);

// Check for the expected log message.
t.true(
target
.getLogger()
.hasMessage("Using configuration file input from repository property"),
);
.withArgs(repositoryProperties)
.logs(t, "Using configuration file input from repository property")
.passes(t.is, repositoryProperties[RepositoryPropertyName.CONFIG_FILE]);
});

test("getConfigFileInput ignores empty repository property value", async (t) => {
// Since the repository property value is an empty/whitespace string, we should ignore it.
await callee(getConfigFileInput)
.withFeatures([Feature.ConfigFileRepositoryProperty])
.withArgs({ [RepositoryPropertyName.CONFIG_FILE]: " " })
.passes(async (fn) => t.is(await fn(), undefined));
.passes(t.is, undefined);
});

test("getConfigFileInput ignores repository property value when FF is off", async (t) => {
// Since the FF is off, we should ignore the repository property value.
const target = callee(getConfigFileInput)
await callee(getConfigFileInput)
.withFeatures([])
.withArgs(repositoryProperties);

await target.passes(async (fn) => t.is(await fn(), undefined));

t.false(
target
.getLogger()
.hasMessage("Using configuration file input from repository property"),
);
t.true(
target
.getLogger()
.hasMessage(
"Ignoring configuration file input from repository property, because the corresponding feature flag is disabled.",
),
);
.withArgs(repositoryProperties)
.notLogs(t, "Using configuration file input from repository property")
.logs(
t,
"Ignoring configuration file input from repository property, because the corresponding feature flag is disabled.",
)
.passes(t.is, undefined);
});
106 changes: 46 additions & 60 deletions src/config/remote-file.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import sinon from "sinon";
import { ActionsEnvVars } from "../environment";
import * as errors from "../error-messages";
import { Feature } from "../feature-flags";
import { callee, getTestEnv } from "../testing-utils";
import { callee } from "../testing-utils";
import { ConfigurationError } from "../util";

import {
Expand Down Expand Up @@ -50,7 +50,7 @@ test("parseRemoteFileAddress accepts full remote addresses", async (t) => {
for (const oldFormatInput of oldFormatInputs) {
await target
.withArgs(oldFormatInput.input)
.passes(async (fn) => t.deepEqual(await fn(), oldFormatInput.expected));
.passes(t.deepEqual, oldFormatInput.expected);
}

// New format.
Expand Down Expand Up @@ -78,28 +78,23 @@ test("parseRemoteFileAddress accepts full remote addresses", async (t) => {
// Should fail when the FF is not enabled.
await targetWithArgs
.withFeatures([])
.passes(async (fn) =>
t.throwsAsync(fn, { instanceOf: ConfigurationError }),
);
.throws(t, { instanceOf: ConfigurationError });

// And pass when the FF is enabled.
await targetWithArgs
.withFeatures([Feature.NewRemoteFileAddresses])
.passes(async (fn) => t.deepEqual(await fn(), newFormatInput.expected));
.passes(t.deepEqual, newFormatInput.expected);
}
});

test("parseRemoteFileAddress accepts remote address without an owner", async (t) => {
const target = callee(parseRemoteFileAddress);

const env = target.getState().env;
const owner = "test-owner";
const getRequired = sinon.stub(env, "getRequired");
getRequired
.withArgs(ActionsEnvVars.GITHUB_REPOSITORY)
.returns(`${owner}/current-repo`);

const targetWithEnv = target.withEnv(env);
const target = callee(parseRemoteFileAddress).withEnv((env) => {
const getRequired = sinon.stub(env, "getRequired");
getRequired
.withArgs(ActionsEnvVars.GITHUB_REPOSITORY)
.returns(`${owner}/current-repo`);
});

const testCases: ParseRemoteFileAddressTest[] = [
{
Expand Down Expand Up @@ -141,33 +136,33 @@ test("parseRemoteFileAddress accepts remote address without an owner", async (t)
];

for (const testCase of testCases) {
const targetWithArgs = targetWithEnv.withArgs(testCase.input);
const targetWithArgs = target.withArgs(testCase.input);

// Should fail when the FF is not enabled.
await targetWithArgs
.withFeatures([])
.passes(async (fn) =>
t.throwsAsync(fn, { instanceOf: ConfigurationError }),
);
.throws(t, { instanceOf: ConfigurationError });

// And pass when the FF is enabled.
await targetWithArgs
.withFeatures([Feature.NewRemoteFileAddresses])
.passes(async (fn) => t.deepEqual(await fn(), testCase.expected));
.passes(t.deepEqual, testCase.expected);
}
});

test("parseRemoteFileAddress throws for invalid `GITHUB_REPOSITORY`", async (t) => {
const target = callee(parseRemoteFileAddress).withArgs("repo@ref");

const env = target.getState().env;
const getRequired = sinon.stub(env, "getRequired");
const getRequired: sinon.SinonStub = sinon.stub();
getRequired.withArgs(ActionsEnvVars.GITHUB_REPOSITORY).returns(`not-valid`);

const target = callee(parseRemoteFileAddress)
.withArgs("repo@ref")
.withEnv((env) => {
sinon.define(env, "getRequired", getRequired);
});
Comment thread
mbg marked this conversation as resolved.

await target
.withEnv(env)
.withFeatures([Feature.NewRemoteFileAddresses])
.passes(async (fn) => t.throwsAsync(fn, { instanceOf: Error }));
.throws(t, { instanceOf: Error });

t.assert(getRequired.calledOnceWith(ActionsEnvVars.GITHUB_REPOSITORY));
});
Expand Down Expand Up @@ -202,43 +197,38 @@ test("parseRemoteFileAddress accepts remote address without a path", async (t) =
// Should fail when the FF is not enabled.
await targetWithArgs
.withFeatures([])
.passes(async (fn) =>
t.throwsAsync(fn, { instanceOf: ConfigurationError }),
);
.throws(t, { instanceOf: ConfigurationError });

// And pass when the FF is enabled.
await targetWithArgs
.withFeatures([Feature.NewRemoteFileAddresses])
.passes(async (fn) => t.deepEqual(await fn(), testCase.expected));
.passes(t.deepEqual, testCase.expected);
}
});

test("parseRemoteFileAddress accepts remote address without a ref", async (t) => {
const target = callee(parseRemoteFileAddress).withArgs("owner/repo:path");

// Should only accept the input if the FF is enabled.
await target.withFeatures([]).passes(t.throwsAsync);
await target.withFeatures([]).throws(t);
await target
.withFeatures([Feature.NewRemoteFileAddresses])
.passes(async (fn) =>
t.deepEqual(await fn(), {
owner: "owner",
repo: "repo",
path: "path",
ref: DEFAULT_CONFIG_FILE_REF,
} satisfies RemoteFileAddress),
);
.passes(t.deepEqual, {
owner: "owner",
repo: "repo",
path: "path",
ref: DEFAULT_CONFIG_FILE_REF,
} satisfies RemoteFileAddress);
});

test("parseRemoteFileAddress rejects invalid values", async (t) => {
const env = getTestEnv();
const owner = "owner";
const getRequired = sinon.stub(env, "getRequired");
getRequired
.withArgs(ActionsEnvVars.GITHUB_REPOSITORY)
.returns(`${owner}/current-repo`);

const target = callee(parseRemoteFileAddress).withEnv(env);
const target = callee(parseRemoteFileAddress).withEnv((env) => {
const getRequired = sinon.stub(env, "getRequired");
getRequired
.withArgs(ActionsEnvVars.GITHUB_REPOSITORY)
.returns(`${owner}/current-repo`);
});

const testInputs = [
" ",
Expand All @@ -262,21 +252,17 @@ test("parseRemoteFileAddress rejects invalid values", async (t) => {
const targetWithArgs = target.withArgs(testInput);

// Should throw both when the new format is and isn't accepted.
await targetWithArgs.withFeatures([]).passes(async (fn) =>
t.throwsAsync(fn, {
instanceOf: ConfigurationError,
message: errors.getConfigFileRepoOldFormatInvalidMessage(testInput),
}),
);
await targetWithArgs.withFeatures([]).throws(t, {
instanceOf: ConfigurationError,
message: errors.getConfigFileRepoOldFormatInvalidMessage(testInput),
});
await targetWithArgs
.withFeatures([Feature.NewRemoteFileAddresses])
.passes(async (fn) =>
t.throwsAsync(fn, {
// When the new format is accepted, there are some more specific
// errors in some cases. It is sufficient for us to check that
// an exception is thrown.
instanceOf: ConfigurationError,
}),
);
.throws(t, {
// When the new format is accepted, there are some more specific
// errors in some cases. It is sufficient for us to check that
// an exception is thrown.
instanceOf: ConfigurationError,
});
}
});
2 changes: 2 additions & 0 deletions src/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,12 @@ export enum ActionsEnvVars {
GITHUB_SERVER_URL = "GITHUB_SERVER_URL",
GITHUB_SHA = "GITHUB_SHA",
GITHUB_WORKFLOW = "GITHUB_WORKFLOW",
GITHUB_WORKSPACE = "GITHUB_WORKSPACE",
RUNNER_ENVIRONMENT = "RUNNER_ENVIRONMENT",
RUNNER_NAME = "RUNNER_NAME",
RUNNER_OS = "RUNNER_OS",
RUNNER_TEMP = "RUNNER_TEMP",
RUNNER_TOOL_CACHE = "RUNNER_TOOL_CACHE",
}

/** A type representing all known environment variables. */
Expand Down
Loading
Loading