From dccdcc151d17e9b86fff87a5c08e8b6768d53fb9 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:34:11 +0200 Subject: [PATCH 01/13] test: improve console matcher Allow using expect matcher for prefix --- test/cli/log.mock.ts | 10 +++++----- test/types/jest.d.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/cli/log.mock.ts b/test/cli/log.mock.ts index 0bf7af25..afbfa5f2 100644 --- a/test/cli/log.mock.ts +++ b/test/cli/log.mock.ts @@ -6,21 +6,21 @@ type LogSpy = jest.MockedFunctionDeep; expect.extend({ toHaveLogLike( spy: LogSpy, - prefix: string, + prefix: string | AsymmetricMatcher, expected: string | AsymmetricMatcher, count: number = 1 ) { - const matches = (s: string) => + const stringMatches = (s: string, expected: string | AsymmetricMatcher) => typeof expected === "string" ? s === expected : expected.asymmetricMatch(s); const calls = spy.mock.calls; - const callsWithPrefix = calls.filter( - ([actualPrefix]) => actualPrefix === prefix + const callsWithPrefix = calls.filter(([actualPrefix]) => + stringMatches(actualPrefix, prefix) ); const matchingCalls = callsWithPrefix.filter(([, actualMessage]) => - matches(actualMessage) + stringMatches(actualMessage, expected) ); const hasMatch = matchingCalls.length >= count; return { diff --git a/test/types/jest.d.ts b/test/types/jest.d.ts index bd83e2f8..2d5bd241 100644 --- a/test/types/jest.d.ts +++ b/test/types/jest.d.ts @@ -31,7 +31,7 @@ declare global { * should have been logged. Defaults to 1 if omitted. */ toHaveLogLike( - prefix: string, + prefix: string | AsymmetricMatcher, expected: string | AsymmetricMatcher, count?: number ): R; From 296d4cc95b32abe50ccb11178160733aacbd5f47 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:34:25 +0200 Subject: [PATCH 02/13] add-cmd --- src/cli/cmd-add.ts | 40 +++++++------- src/cli/error-logging.ts | 2 +- src/cli/index.ts | 4 +- src/cli/result-codes.ts | 14 +++++ test/cli/cmd-add.test.ts | 111 ++++++++++++++++++++++----------------- 5 files changed, 100 insertions(+), 71 deletions(-) create mode 100644 src/cli/result-codes.ts diff --git a/src/cli/cmd-add.ts b/src/cli/cmd-add.ts index a0cb27b7..e8368079 100644 --- a/src/cli/cmd-add.ts +++ b/src/cli/cmd-add.ts @@ -1,11 +1,9 @@ import { isPackageUrl, PackageUrl } from "../domain/package-url"; import { LoadProjectManifest, - ManifestLoadError, - ManifestWriteError, WriteProjectManifest, } from "../io/project-manifest-io"; -import { EnvParseError, ParseEnvService } from "../services/parse-env"; +import { ParseEnvService } from "../services/parse-env"; import { compareEditorVersion, stringifyEditorVersion, @@ -53,9 +51,10 @@ import { logValidDependency } from "./dependency-logging"; import { unityRegistryUrl } from "../domain/registry-url"; import { tryGetTargetEditorVersionFor } from "../domain/package-manifest"; import { VersionNotFoundError } from "../domain/packument"; -import { FetchPackumentError } from "../io/packument-io"; import { DebugLog } from "../logging"; import { DetermineEditorVersion } from "../services/determine-editor-version"; +import { FetchPackumentError } from "../io/packument-io"; +import { ResultCodes } from "./result-codes"; export class InvalidPackumentDataError extends CustomError { private readonly _class = "InvalidPackumentDataError"; @@ -85,16 +84,18 @@ export type AddOptions = CmdOptions<{ force?: boolean; }>; -export type AddError = - | EnvParseError - | ManifestLoadError +type AddError = | PackumentVersionResolveError | FetchPackumentError | InvalidPackumentDataError | EditorIncompatibleError | UnresolvedDependencyError - | DependencyResolveError - | ManifestWriteError; + | DependencyResolveError; + +/** + * The different command result codes for the add command. + */ +export type AddResultCode = ResultCodes.Ok | ResultCodes.Error; /** * Cmd-handler for adding packages. @@ -104,7 +105,7 @@ export type AddError = type AddCmd = ( pkgs: PackageReference | PackageReference[], options: AddOptions -) => Promise>; +) => Promise; /** * Makes a {@link AddCmd} function. @@ -121,18 +122,19 @@ export function makeAddCmd( ): AddCmd { return async (pkgs, options) => { if (!Array.isArray(pkgs)) pkgs = [pkgs]; + // parse env const envResult = await parseEnv(options); if (envResult.isErr()) { logEnvParseError(log, envResult.error); - return envResult; + return ResultCodes.Error; } const env = envResult.value; const editorVersionResult = await determineEditorVersion(env.cwd).promise; if (editorVersionResult.isErr()) { logDetermineEditorError(log, editorVersionResult.error); - return editorVersionResult; + return ResultCodes.Error; } const editorVersion = editorVersionResult.value; @@ -232,10 +234,10 @@ export function makeAddCmd( requestedVersion, true ); - if (resolveResult.isErr()) - // TODO: Log errors - // TODO: Add tests + if (resolveResult.isErr()) { + logPackumentResolveError(log, name, resolveResult.error); return resolveResult; + } const [depsValid, depsInvalid] = resolveResult.value; // add depsValid to pkgsInScope. @@ -331,7 +333,7 @@ export function makeAddCmd( const loadResult = await loadProjectManifest(env.cwd).promise; if (loadResult.isErr()) { logManifestLoadError(log, loadResult.error); - return loadResult; + return ResultCodes.Error; } let manifest = loadResult.value; @@ -339,7 +341,7 @@ export function makeAddCmd( let dirty = false; for (const pkg of pkgs) { const result = await tryAddToManifest(manifest, pkg); - if (result.isErr()) return result; + if (result.isErr()) return ResultCodes.Error; const [newManifest, manifestChanged] = result.value; if (manifestChanged) { @@ -353,13 +355,13 @@ export function makeAddCmd( const saveResult = await writeProjectManifest(env.cwd, manifest).promise; if (saveResult.isErr()) { logManifestSaveError(log, saveResult.error); - return saveResult; + return ResultCodes.Error; } // print manifest notice log.notice("", "please open Unity project to apply changes"); } - return Ok(undefined); + return ResultCodes.Ok; }; } diff --git a/src/cli/error-logging.ts b/src/cli/error-logging.ts index ae8dff4b..ede5a6d5 100644 --- a/src/cli/error-logging.ts +++ b/src/cli/error-logging.ts @@ -29,7 +29,7 @@ export function logManifestLoadError(log: Logger, error: ManifestLoadError) { : "the manifest file did not contain a valid project manifest"; const prefix = "manifest"; - const errorMessage = `Could not load project manifest because ${reason}.`; + const errorMessage = `could not load project manifest because ${reason}.`; log.error(prefix, errorMessage); // TODO: Print fix suggestions diff --git a/src/cli/index.ts b/src/cli/index.ts index 7704a2c5..824eb9ce 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -179,8 +179,8 @@ openupm add @ [otherPkgs...]` ) .action(async function (pkg, otherPkgs, options) { const pkgs = [pkg].concat(otherPkgs); - const addResult = await addCmd(pkgs, makeCmdOptions(options)); - if (addResult.isErr()) process.exit(1); + const resultCode = await addCmd(pkgs, makeCmdOptions(options)); + process.exit(resultCode); }); program diff --git a/src/cli/result-codes.ts b/src/cli/result-codes.ts new file mode 100644 index 00000000..595cb01f --- /dev/null +++ b/src/cli/result-codes.ts @@ -0,0 +1,14 @@ +/** + * The set of all result-codes with which openupm can exit. + */ +export enum ResultCodes { + // TODO: Add more differentiated error codes + /** + * The operation completed successfully. + */ + Ok = 0, + /** + * The operation failed. + */ + Error = 1, +} diff --git a/test/cli/cmd-add.test.ts b/test/cli/cmd-add.test.ts index 24a9b368..23707bbf 100644 --- a/test/cli/cmd-add.test.ts +++ b/test/cli/cmd-add.test.ts @@ -1,15 +1,9 @@ -import { - EditorIncompatibleError, - InvalidPackumentDataError, - makeAddCmd, - UnresolvedDependencyError, -} from "../../src/cli/cmd-add"; +import { makeAddCmd } from "../../src/cli/cmd-add"; import { makeDomainName } from "../../src/domain/domain-name"; import { Env, ParseEnvService } from "../../src/services/parse-env"; import { exampleRegistryUrl } from "../domain/data-registry"; import { unityRegistryUrl } from "../../src/domain/registry-url"; import { makeEditorVersion } from "../../src/domain/editor-version"; -import { Err, Ok } from "ts-results-es"; import { mockProjectManifest, mockProjectManifestWriteResult, @@ -18,7 +12,6 @@ import { emptyProjectManifest } from "../../src/domain/project-manifest"; import { makeMockLogger } from "./log.mock"; import { buildPackument } from "../domain/data-packument"; import { mockResolvedPackuments } from "../services/packument-resolving.mock"; -import { PackumentNotFoundError } from "../../src/common-errors"; import { buildProjectManifest } from "../domain/data-project-manifest"; import { ResolveDependenciesService } from "../../src/services/dependency-resolving"; import { makeSemanticVersion } from "../../src/domain/semantic-version"; @@ -31,8 +24,10 @@ import { import { makePackageReference } from "../../src/domain/package-reference"; import { VersionNotFoundError } from "../../src/domain/packument"; import { noopLogger } from "../../src/logging"; -import { FileMissingError, GenericIOError } from "../../src/io/common-errors"; +import { GenericIOError } from "../../src/io/common-errors"; import { DetermineEditorVersion } from "../../src/services/determine-editor-version"; +import { Err, Ok } from "ts-results-es"; +import { ResultCodes } from "../../src/cli/result-codes"; const somePackage = makeDomainName("com.some.package"); const otherPackage = makeDomainName("com.other.package"); @@ -136,9 +131,21 @@ describe("cmd-add", () => { const { addCmd, parseEnv } = makeDependencies(); parseEnv.mockResolvedValue(Err(expected)); - const result = await addCmd(somePackage, { _global: {} }); + const resultCode = await addCmd(somePackage, { _global: {} }); + + expect(resultCode).toEqual(ResultCodes.Error); + }); + + it("should notify if env could not be parsed", async () => { + const { addCmd, parseEnv, log } = makeDependencies(); + parseEnv.mockResolvedValue(Err(new GenericIOError())); + + await addCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("environment information could not be parsed") + ); }); it("should fail if editor-version could not be determined", async () => { @@ -147,10 +154,22 @@ describe("cmd-add", () => { Err(new GenericIOError()).toAsyncResult() ); - const result = await addCmd(somePackage, { _global: {} }); + const resultCode = await addCmd(somePackage, { _global: {} }); + + expect(resultCode).toEqual(ResultCodes.Error); + }); + + it("should notify if editor-version could not be determined", async () => { + const { addCmd, determineEditorVersion, log } = makeDependencies(); + determineEditorVersion.mockReturnValue( + Err(new GenericIOError()).toAsyncResult() + ); + + await addCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => - expect(actual).toBeInstanceOf(GenericIOError) + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("editor version could be determined") ); }); @@ -158,11 +177,9 @@ describe("cmd-add", () => { const { addCmd, loadProjectManifest } = makeDependencies(); mockProjectManifest(loadProjectManifest, null); - const result = await addCmd(somePackage, { _global: {} }); + const resultCode = await addCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => - expect(actual).toBeInstanceOf(FileMissingError) - ); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if manifest could not be loaded", async () => { @@ -171,18 +188,19 @@ describe("cmd-add", () => { await addCmd(somePackage, { _global: {} }); - expect(log.error).toHaveLogLike("manifest", expect.any(String)); + expect(log.error).toHaveLogLike( + expect.any(String), + expect.stringContaining("could not load project manifest") + ); }); it("should fail if package could not be resolved", async () => { const { addCmd, resolveRemovePackumentVersion } = makeDependencies(); mockResolvedPackuments(resolveRemovePackumentVersion); - const result = await addCmd(somePackage, { _global: {} }); + const resultCode = await addCmd(somePackage, { _global: {} }); - expect(result).toBeError((error) => - expect(error).toBeInstanceOf(PackumentNotFoundError) - ); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if package could not be resolved", async () => { @@ -200,13 +218,14 @@ describe("cmd-add", () => { it("should fail if package version could not be resolved", async () => { const { addCmd } = makeDependencies(); - const result = await addCmd(makePackageReference(somePackage, "2.0.0"), { - _global: {}, - }); - - expect(result).toBeError((error) => - expect(error).toBeInstanceOf(VersionNotFoundError) + const resultCode = await addCmd( + makePackageReference(somePackage, "2.0.0"), + { + _global: {}, + } ); + + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if package version could not be resolved", async () => { @@ -277,13 +296,11 @@ describe("cmd-add", () => { badEditorPackument, ]); - const result = await addCmd(somePackage, { + const resultCode = await addCmd(somePackage, { _global: {}, }); - expect(result).toBeError((error) => - expect(error).toBeInstanceOf(InvalidPackumentDataError) - ); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should add package with invalid editor version when running with force", async () => { @@ -293,12 +310,12 @@ describe("cmd-add", () => { badEditorPackument, ]); - const result = await addCmd(somePackage, { + const resultCode = await addCmd(somePackage, { _global: {}, force: true, }); - expect(result).toBeOk(); + expect(resultCode).toEqual(ResultCodes.Ok); }); it("should notify if package is incompatible with editor", async () => { @@ -342,13 +359,11 @@ describe("cmd-add", () => { incompatiblePackument, ]); - const result = await addCmd(somePackage, { + const resultCode = await addCmd(somePackage, { _global: {}, }); - expect(result).toBeError((error) => - expect(error).toBeInstanceOf(EditorIncompatibleError) - ); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should add package with incompatible with editor when running with force", async () => { @@ -358,12 +373,12 @@ describe("cmd-add", () => { incompatiblePackument, ]); - const result = await addCmd(somePackage, { + const resultCode = await addCmd(somePackage, { _global: {}, force: true, }); - expect(result).toBeOk(); + expect(resultCode).toEqual(ResultCodes.Ok); }); it("should notify of unresolved dependencies", async () => { @@ -471,13 +486,11 @@ describe("cmd-add", () => { ]) ); - const result = await addCmd(somePackage, { + const resultCode = await addCmd(somePackage, { _global: {}, }); - expect(result).toBeError((error) => - expect(error).toBeInstanceOf(UnresolvedDependencyError) - ); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should add package with unresolved dependency when running with force", async () => { @@ -495,12 +508,12 @@ describe("cmd-add", () => { ]) ); - const result = await addCmd(somePackage, { + const resultCode = await addCmd(somePackage, { _global: {}, force: true, }); - expect(result).toBeOk(); + expect(resultCode).toEqual(ResultCodes.Ok); }); it("should add package", async () => { @@ -675,9 +688,9 @@ describe("cmd-add", () => { const { addCmd, writeProjectManifest } = makeDependencies(); mockProjectManifestWriteResult(writeProjectManifest, expected); - const result = await addCmd(somePackage, { _global: {} }); + const resultCode = await addCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if manifest could not be saved", async () => { From e2f7ff87ee3bf63346d5cfbed668b3c00883d0da Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:37:51 +0200 Subject: [PATCH 03/13] deps-cmd --- src/cli/cmd-deps.ts | 33 +++++++++++++++++---------------- src/cli/index.ts | 4 ++-- test/cli/cmd-deps.test.ts | 3 ++- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/cli/cmd-deps.ts b/src/cli/cmd-deps.ts index 0bf735ff..97975588 100644 --- a/src/cli/cmd-deps.ts +++ b/src/cli/cmd-deps.ts @@ -1,4 +1,4 @@ -import { EnvParseError, ParseEnvService } from "../services/parse-env"; +import { ParseEnvService } from "../services/parse-env"; import { isPackageUrl } from "../domain/package-url"; import { makePackageReference, @@ -8,23 +8,23 @@ import { import { CmdOptions } from "./options"; import { PackumentVersionResolveError } from "../packument-version-resolving"; import { PackumentNotFoundError } from "../common-errors"; -import { Ok, Result } from "ts-results-es"; -import { - DependencyResolveError, - ResolveDependenciesService, -} from "../services/dependency-resolving"; +import { ResolveDependenciesService } from "../services/dependency-resolving"; import { Logger } from "npmlog"; import { logValidDependency } from "./dependency-logging"; import { VersionNotFoundError } from "../domain/packument"; -import { logEnvParseError } from "./error-logging"; +import { logEnvParseError, logPackumentResolveError } from "./error-logging"; import { DebugLog } from "../logging"; - -export type DepsError = EnvParseError | DependencyResolveError; +import { ResultCodes } from "./result-codes"; export type DepsOptions = CmdOptions<{ deep?: boolean; }>; +/** + * The possible result codes with which the deps command can exit. + */ +export type DepsResultCode = ResultCodes.Ok | ResultCodes.Error; + /** * Cmd-handler for listing dependencies for a package. * @param pkg Reference to a package. @@ -33,7 +33,7 @@ export type DepsOptions = CmdOptions<{ export type DepsCmd = ( pkg: PackageReference, options: DepsOptions -) => Promise>; +) => Promise; function errorPrefixForError(error: PackumentVersionResolveError): string { if (error instanceof PackumentNotFoundError) return "missing dependency"; @@ -56,7 +56,7 @@ export function makeDepsCmd( const envResult = await parseEnv(options); if (envResult.isErr()) { logEnvParseError(log, envResult.error); - return envResult; + return ResultCodes.Error; } const env = envResult.value; @@ -74,10 +74,11 @@ export function makeDepsCmd( version, deep ); - if (resolveResult.isErr()) - // TODO: Log errors - // TODO: Add tests - return resolveResult; + if (resolveResult.isErr()) { + logPackumentResolveError(log, name, resolveResult.error); + return ResultCodes.Error; + } + const [depsValid, depsInvalid] = resolveResult.value; depsValid.forEach((dependency) => logValidDependency(debugLog, dependency)); @@ -93,6 +94,6 @@ export function makeDepsCmd( log.warn(prefix, x.name); }); - return Ok(undefined); + return ResultCodes.Ok; }; } diff --git a/src/cli/index.ts b/src/cli/index.ts index 824eb9ce..ff592ff9 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -230,8 +230,8 @@ openupm deps openupm deps @` ) .action(async function (pkg, options) { - const depsResult = await depsCmd(pkg, makeCmdOptions(options)); - if (depsResult.isErr()) process.exit(1); + const resultCode = await depsCmd(pkg, makeCmdOptions(options)); + process.exit(resultCode); }); program diff --git a/test/cli/cmd-deps.test.ts b/test/cli/cmd-deps.test.ts index 466262a5..a71ab676 100644 --- a/test/cli/cmd-deps.test.ts +++ b/test/cli/cmd-deps.test.ts @@ -13,6 +13,7 @@ import { mockService } from "../services/service.mock"; import { VersionNotFoundError } from "../../src/domain/packument"; import { noopLogger } from "../../src/logging"; import { GenericIOError } from "../../src/io/common-errors"; +import {ResultCodes} from "../../src/cli/result-codes"; const somePackage = makeDomainName("com.some.package"); const otherPackage = makeDomainName("com.other.package"); @@ -61,7 +62,7 @@ describe("cmd-deps", () => { const result = await depsCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toEqual(ResultCodes.Error); }); it("should fail if package-reference has url-version", async () => { From 7b962780d514ee1cba358e66a49ec65a4eb62979 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:40:43 +0200 Subject: [PATCH 04/13] login-cmd --- src/cli/cmd-login.ts | 40 ++++++++++++++++---------------------- src/cli/index.ts | 4 ++-- test/cli/cmd-login.test.ts | 7 ++++--- 3 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/cli/cmd-login.ts b/src/cli/cmd-login.ts index f27e3884..7d4a3749 100644 --- a/src/cli/cmd-login.ts +++ b/src/cli/cmd-login.ts @@ -1,6 +1,6 @@ import { AuthenticationError } from "../services/npm-login"; -import { GetUpmConfigPath, GetUpmConfigPathError } from "../io/upm-config-io"; -import { EnvParseError, ParseEnvService } from "../services/parse-env"; +import { GetUpmConfigPath } from "../io/upm-config-io"; +import { ParseEnvService } from "../services/parse-env"; import { coerceRegistryUrl } from "../domain/registry-url"; import { promptEmail, @@ -9,23 +9,10 @@ import { promptUsername, } from "./prompts"; import { CmdOptions } from "./options"; -import { Ok, Result } from "ts-results-es"; -import { NpmrcLoadError, NpmrcSaveError } from "../io/npmrc-io"; import { Logger } from "npmlog"; -import { UpmAuthStoreError } from "../services/upm-auth"; import { LoginService } from "../services/login"; import { logEnvParseError } from "./error-logging"; - -/** - * Errors which may occur when logging in. - */ -export type LoginError = - | EnvParseError - | GetUpmConfigPathError - | AuthenticationError - | NpmrcLoadError - | NpmrcSaveError - | UpmAuthStoreError; +import { ResultCodes } from "./result-codes"; /** * Options for logging in a user. These come from the CLI. @@ -40,13 +27,16 @@ export type LoginOptions = CmdOptions<{ alwaysAuth?: boolean; }>; +/** + * The possible result codes with which the login command can exit. + */ +export type LoginResultCode = ResultCodes.Ok | ResultCodes.Error; + /** * Cmd-handler for logging in users. * @param options Options for logging in. */ -export type LoginCmd = ( - options: LoginOptions -) => Promise>; +export type LoginCmd = (options: LoginOptions) => Promise; /** * Makes a {@link LoginCmd} function. @@ -62,7 +52,7 @@ export function makeLoginCmd( const envResult = await parseEnv(options); if (envResult.isErr()) { logEnvParseError(log, envResult.error); - return envResult; + return ResultCodes.Error; } const env = envResult.value; @@ -80,7 +70,10 @@ export function makeLoginCmd( const configPathResult = await getUpmConfigPath(env.wsl, env.systemUser) .promise; - if (configPathResult.isErr()) return configPathResult; + if (configPathResult.isErr()) { + // TODO: Log error + return ResultCodes.Error; + } const configPath = configPathResult.value; const loginResult = await login( @@ -101,11 +94,12 @@ export function makeLoginCmd( else log.error(loginError.status.toString(), loginError.message); } - return loginResult; + // TODO: Log all errors + return ResultCodes.Error; } log.notice("auth", `you are authenticated as '${username}'`); log.notice("config", "saved unity config at " + configPath); - return Ok(undefined); + return ResultCodes.Ok; }; } diff --git a/src/cli/index.ts b/src/cli/index.ts index ff592ff9..736805d5 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -247,8 +247,8 @@ program ) .description("authenticate with a scoped registry") .action(async function (options) { - const loginResult = await loginCmd(makeCmdOptions(options)); - if (loginResult.isErr()) process.exit(1); + const resultCode = await loginCmd(makeCmdOptions(options)); + process.exit(resultCode); }); // prompt for invalid command diff --git a/test/cli/cmd-login.test.ts b/test/cli/cmd-login.test.ts index fabf6606..3515cbc7 100644 --- a/test/cli/cmd-login.test.ts +++ b/test/cli/cmd-login.test.ts @@ -12,6 +12,7 @@ import { exampleRegistryUrl } from "../domain/data-registry"; import { unityRegistryUrl } from "../../src/domain/registry-url"; import { AuthenticationError } from "../../src/services/npm-login"; import { GenericIOError } from "../../src/io/common-errors"; +import { ResultCodes } from "../../src/cli/result-codes"; const defaultEnv = { cwd: "/users/some-user/projects/SomeProject", @@ -48,7 +49,7 @@ describe("cmd-login", () => { const result = await loginCmd({ _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toEqual(ResultCodes.Error); }); // TODO: Add tests for prompting logic @@ -65,7 +66,7 @@ describe("cmd-login", () => { _global: { registry: exampleRegistryUrl }, }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toEqual(ResultCodes.Error); }); it("should fail if login failed", async () => { @@ -80,7 +81,7 @@ describe("cmd-login", () => { _global: { registry: exampleRegistryUrl }, }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toEqual(ResultCodes.Error); }); it("should notify if unauthorized", async () => { From 95621da5353018550abe71d456b2ae54ff15fc1c Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:45:07 +0200 Subject: [PATCH 05/13] remove-cmd --- src/cli/cmd-remove.ts | 20 +++++++++++++------- src/cli/index.ts | 4 ++-- test/cli/cmd-remove.test.ts | 25 ++++++++----------------- 3 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/cli/cmd-remove.ts b/src/cli/cmd-remove.ts index b0d6c35a..f682abcc 100644 --- a/src/cli/cmd-remove.ts +++ b/src/cli/cmd-remove.ts @@ -30,8 +30,14 @@ import { logManifestSaveError, } from "./error-logging"; import { Logger } from "npmlog"; +import { ResultCodes } from "./result-codes"; -export type RemoveError = +/** + * The possible result codes with which the remove command can exit. + */ +export type RemoveResultCode = ResultCodes.Ok | ResultCodes.Error; + +type RemoveError = | EnvParseError | PackageWithVersionError | PackumentNotFoundError @@ -48,7 +54,7 @@ export type RemoveOptions = CmdOptions; export type RemoveCmd = ( pkgs: PackageReference[] | PackageReference, options: RemoveOptions -) => Promise>; +) => Promise; /** * Makes a {@link RemoveCmd} function. @@ -65,7 +71,7 @@ export function makeRemoveCmd( const envResult = await parseEnv(options); if (envResult.isErr()) { logEnvParseError(log, envResult.error); - return envResult; + return ResultCodes.Error; } const env = envResult.value; @@ -105,14 +111,14 @@ export function makeRemoveCmd( const manifestResult = await loadProjectManifest(env.cwd).promise; if (manifestResult.isErr()) { logManifestLoadError(log, manifestResult.error); - return manifestResult; + return ResultCodes.Error; } let manifest = manifestResult.value; // remove for (const pkg of pkgs) { const result = await tryRemoveFromManifest(manifest, pkg); - if (result.isErr()) return result; + if (result.isErr()) return ResultCodes.Error; manifest = result.value; } @@ -120,12 +126,12 @@ export function makeRemoveCmd( const saveResult = await writeProjectManifest(env.cwd, manifest).promise; if (saveResult.isErr()) { logManifestSaveError(log, saveResult.error); - return saveResult; + return ResultCodes.Error; } // print manifest notice log.notice("", "please open Unity project to apply changes"); - return Ok(undefined); + return ResultCodes.Ok; }; } diff --git a/src/cli/index.ts b/src/cli/index.ts index 736805d5..d17b33ea 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -195,8 +195,8 @@ program .description("remove package from manifest json") .action(async function (pkg, otherPkgs, options) { const pkgs = [pkg].concat(otherPkgs); - const removeResult = await removeCmd(pkgs, makeCmdOptions(options)); - if (removeResult.isErr()) process.exit(1); + const resultCode = await removeCmd(pkgs, makeCmdOptions(options)); + process.exit(resultCode); }); program diff --git a/test/cli/cmd-remove.test.ts b/test/cli/cmd-remove.test.ts index c8d7af01..7a24e215 100644 --- a/test/cli/cmd-remove.test.ts +++ b/test/cli/cmd-remove.test.ts @@ -1,6 +1,6 @@ import { exampleRegistryUrl } from "../domain/data-registry"; import { Env, ParseEnvService } from "../../src/services/parse-env"; -import { makeRemoveCmd, RemoveError } from "../../src/cli/cmd-remove"; +import { makeRemoveCmd } from "../../src/cli/cmd-remove"; import { Err, Ok } from "ts-results-es"; import { makeDomainName } from "../../src/domain/domain-name"; import { @@ -10,17 +10,14 @@ import { import { buildProjectManifest } from "../domain/data-project-manifest"; import { makePackageReference } from "../../src/domain/package-reference"; import { makeSemanticVersion } from "../../src/domain/semantic-version"; -import { - PackageWithVersionError, - PackumentNotFoundError, -} from "../../src/common-errors"; import { makeMockLogger } from "./log.mock"; import { mockService } from "../services/service.mock"; import { LoadProjectManifest, WriteProjectManifest, } from "../../src/io/project-manifest-io"; -import { FileMissingError, GenericIOError } from "../../src/io/common-errors"; +import { GenericIOError } from "../../src/io/common-errors"; +import { ResultCodes } from "../../src/cli/result-codes"; const somePackage = makeDomainName("com.some.package"); const otherPackage = makeDomainName("com.other.package"); @@ -67,7 +64,7 @@ describe("cmd-remove", () => { const result = await removeCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toEqual(ResultCodes.Error); }); it("should fail if manifest could not be loaded", async () => { @@ -76,9 +73,7 @@ describe("cmd-remove", () => { const result = await removeCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual: RemoveError) => - expect(actual).toBeInstanceOf(FileMissingError) - ); + expect(result).toEqual(ResultCodes.Error); }); it("should notify if manifest could not be loaded", async () => { @@ -98,9 +93,7 @@ describe("cmd-remove", () => { { _global: {} } ); - expect(result).toBeError((actual) => - expect(actual).toBeInstanceOf(PackageWithVersionError) - ); + expect(result).toEqual(ResultCodes.Error); }); it("should notify if package version was specified", async () => { @@ -122,9 +115,7 @@ describe("cmd-remove", () => { const result = await removeCmd(otherPackage, { _global: {} }); - expect(result).toBeError((actual) => - expect(actual).toBeInstanceOf(PackumentNotFoundError) - ); + expect(result).toEqual(ResultCodes.Error); }); it("should notify if package is not in manifest", async () => { @@ -193,7 +184,7 @@ describe("cmd-remove", () => { const result = await removeCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toEqual(ResultCodes.Error); }); it("should notify if manifest could not be saved", async () => { From c6a1e45a9cc36cb0df86dbe2496b713e8cb37cd1 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:47:02 +0200 Subject: [PATCH 06/13] search-cmd --- src/cli/cmd-search.ts | 20 +++++++++++--------- src/cli/index.ts | 4 ++-- test/cli/cmd-search.test.ts | 5 +++-- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/cli/cmd-search.ts b/src/cli/cmd-search.ts index 5ed6c051..97f55dc8 100644 --- a/src/cli/cmd-search.ts +++ b/src/cli/cmd-search.ts @@ -1,15 +1,17 @@ import * as os from "os"; -import { EnvParseError, ParseEnvService } from "../services/parse-env"; +import { ParseEnvService } from "../services/parse-env"; import { CmdOptions } from "./options"; import { formatAsTable } from "./output-formatting"; -import { Ok, Result } from "ts-results-es"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { Logger } from "npmlog"; import { SearchPackages } from "../services/search-packages"; import { logEnvParseError } from "./error-logging"; import { DebugLog } from "../logging"; +import { ResultCodes } from "./result-codes"; -export type SearchError = EnvParseError | HttpErrorBase; +/** + * The possible result codes with which the search command can exit. + */ +export type SearchResultCode = ResultCodes.Ok | ResultCodes.Error; export type SearchOptions = CmdOptions; @@ -21,7 +23,7 @@ export type SearchOptions = CmdOptions; export type SearchCmd = ( keyword: string, options: SearchOptions -) => Promise>; +) => Promise; /** * Makes a {@link SearchCmd} function. @@ -37,7 +39,7 @@ export function makeSearchCmd( const envResult = await parseEnv(options); if (envResult.isErr()) { logEnvParseError(log, envResult.error); - return envResult; + return ResultCodes.Error; } const env = envResult.value; @@ -49,17 +51,17 @@ export function makeSearchCmd( if (searchResult.isErr()) { log.warn("", "/-/all endpoint is not available"); - return searchResult; + return ResultCodes.Error; } const results = searchResult.value; if (results.length === 0) { log.notice("", `No matches found for "${keyword}"`); - return Ok(undefined); + return ResultCodes.Ok; } debugLog(`${usedEndpoint}: ${results.map((it) => it.name).join(os.EOL)}`); console.log(formatAsTable(results)); - return Ok(undefined); + return ResultCodes.Ok; }; } diff --git a/src/cli/index.ts b/src/cli/index.ts index d17b33ea..5ba0d4da 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -205,8 +205,8 @@ program .aliases(["s", "se", "find"]) .description("Search package by keyword") .action(async function (keyword, options) { - const searchResult = await searchCmd(keyword, makeCmdOptions(options)); - if (searchResult.isErr()) process.exit(1); + const resultCode = await searchCmd(keyword, makeCmdOptions(options)); + process.exit(resultCode); }); program diff --git a/test/cli/cmd-search.test.ts b/test/cli/cmd-search.test.ts index e0a4c7b9..7b9a1526 100644 --- a/test/cli/cmd-search.test.ts +++ b/test/cli/cmd-search.test.ts @@ -10,6 +10,7 @@ import { mockService } from "../services/service.mock"; import { SearchPackages } from "../../src/services/search-packages"; import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { noopLogger } from "../../src/logging"; +import { ResultCodes } from "../../src/cli/result-codes"; const exampleSearchResult: SearchedPackument = { name: makeDomainName("com.example.package-a"), @@ -67,7 +68,7 @@ describe("cmd-search", () => { const result = await searchCmd("pkg-not-exist", options); - expect(result).toBeOk(); + expect(result).toEqual(ResultCodes.Ok); }); it("should notify of unknown packument", async () => { @@ -89,7 +90,7 @@ describe("cmd-search", () => { const result = await searchCmd("package-a", options); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toEqual(ResultCodes.Error); }); it("should notify if packuments could not be searched", async () => { From c209bf330be6cd0044c1cb8568aa72edb09cbeaa Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:50:39 +0200 Subject: [PATCH 07/13] view-cmd --- src/cli/cmd-view.ts | 28 +++++++++++++++------------- src/cli/index.ts | 4 ++-- test/cli/cmd-view.test.ts | 24 +++++++++--------------- 3 files changed, 26 insertions(+), 30 deletions(-) diff --git a/src/cli/cmd-view.ts b/src/cli/cmd-view.ts index daeaba1f..4f06318e 100644 --- a/src/cli/cmd-view.ts +++ b/src/cli/cmd-view.ts @@ -1,7 +1,7 @@ import chalk from "chalk"; import assert from "assert"; import { tryGetLatestVersion, UnityPackument } from "../domain/packument"; -import { EnvParseError, ParseEnvService } from "../services/parse-env"; +import { ParseEnvService } from "../services/parse-env"; import { hasVersion, PackageReference, @@ -9,18 +9,17 @@ import { } from "../domain/package-reference"; import { CmdOptions } from "./options"; import { recordKeys } from "../utils/record-utils"; -import { Err, Ok, Result } from "ts-results-es"; -import { - PackageWithVersionError, - PackumentNotFoundError, -} from "../common-errors"; import { Logger } from "npmlog"; import { ResolveRemotePackument } from "../services/resolve-remote-packument"; import { logEnvParseError } from "./error-logging"; +import { ResultCodes } from "./result-codes"; export type ViewOptions = CmdOptions; -export type ViewError = EnvParseError | PackageWithVersionError; +/** + * The possible result codes with which the view command can exit. + */ +export type ViewResultCode = ResultCodes.Ok | ResultCodes.Error; /** * Cmd-handler for viewing package information. @@ -30,7 +29,7 @@ export type ViewError = EnvParseError | PackageWithVersionError; export type ViewCmd = ( pkg: PackageReference, options: ViewOptions -) => Promise>; +) => Promise; const printInfo = function (packument: UnityPackument) { const versionCount = recordKeys(packument.versions).length; @@ -114,7 +113,7 @@ export function makeViewCmd( const envResult = await parseEnv(options); if (envResult.isErr()) { logEnvParseError(log, envResult.error); - return envResult; + return ResultCodes.Error; } const env = envResult.value; @@ -122,7 +121,7 @@ export function makeViewCmd( if (hasVersion(pkg)) { const [name] = splitPackageReference(pkg); log.warn("", `please do not specify a version (Write only '${name}').`); - return Err(new PackageWithVersionError()); + return ResultCodes.Error; } // verify name @@ -131,15 +130,18 @@ export function makeViewCmd( ...(env.upstream ? [env.upstreamRegistry] : []), ]; const resolveResult = await resolveRemotePackument(pkg, sources).promise; - if (!resolveResult.isOk()) return resolveResult; + if (!resolveResult.isOk()) { + // TODO: Print error + return ResultCodes.Error; + } const packument = resolveResult.value?.packument ?? null; if (packument === null) { log.error("404", `package not found: ${pkg}`); - return Err(new PackumentNotFoundError()); + return ResultCodes.Error; } printInfo(packument); - return Ok(undefined); + return ResultCodes.Ok; }; } diff --git a/src/cli/index.ts b/src/cli/index.ts index 5ba0d4da..6d9eb87d 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -215,8 +215,8 @@ program .aliases(["v", "info", "show"]) .description("view package information") .action(async function (pkg, options) { - const result = await viewCmd(pkg, makeCmdOptions(options)); - if (result.isErr()) process.exit(1); + const resultCode = await viewCmd(pkg, makeCmdOptions(options)); + process.exit(resultCode); }); program diff --git a/test/cli/cmd-view.test.ts b/test/cli/cmd-view.test.ts index 0a16dd3d..6992d10d 100644 --- a/test/cli/cmd-view.test.ts +++ b/test/cli/cmd-view.test.ts @@ -6,16 +6,13 @@ import { Err, Ok } from "ts-results-es"; import { makeDomainName } from "../../src/domain/domain-name"; import { makePackageReference } from "../../src/domain/package-reference"; import { makeSemanticVersion } from "../../src/domain/semantic-version"; -import { - PackageWithVersionError, - PackumentNotFoundError, -} from "../../src/common-errors"; import { makeMockLogger } from "./log.mock"; import { buildPackument } from "../domain/data-packument"; import { mockService } from "../services/service.mock"; import { ResolveRemotePackument } from "../../src/services/resolve-remote-packument"; import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { GenericIOError } from "../../src/io/common-errors"; +import { ResultCodes } from "../../src/cli/result-codes"; const somePackage = makeDomainName("com.some.package"); const somePackument = buildPackument(somePackage, (packument) => @@ -75,22 +72,20 @@ describe("cmd-view", () => { const { viewCmd, parseEnv } = makeDependencies(); parseEnv.mockResolvedValue(Err(expected)); - const result = await viewCmd(somePackage, { _global: {} }); + const resultCode = await viewCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should fail if package version was specified", async () => { const { viewCmd } = makeDependencies(); - const result = await viewCmd( + const resultCode = await viewCmd( makePackageReference(somePackage, makeSemanticVersion("1.0.0")), { _global: {} } ); - expect(result).toBeError((actual) => - expect(actual).toBeInstanceOf(PackageWithVersionError) - ); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if package version was specified", async () => { @@ -108,13 +103,12 @@ describe("cmd-view", () => { }); it("should fail if package was not found", async () => { - const expected = new PackumentNotFoundError(); const { viewCmd, resolveRemotePackument } = makeDependencies(); resolveRemotePackument.mockReturnValue(Ok(null).toAsyncResult()); - const result = await viewCmd(somePackage, { _global: {} }); + const resultCode = await viewCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should fail if package could not be resolved", async () => { @@ -122,9 +116,9 @@ describe("cmd-view", () => { const { viewCmd, resolveRemotePackument } = makeDependencies(); resolveRemotePackument.mockReturnValue(Err(expected).toAsyncResult()); - const result = await viewCmd(somePackage, { _global: {} }); + const resultCode = await viewCmd(somePackage, { _global: {} }); - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if package could not be resolved", async () => { From 7d737693e3f705155ffd60a7834c1f93574bac43 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:50:47 +0200 Subject: [PATCH 08/13] rename variables --- test/cli/cmd-deps.test.ts | 6 +++--- test/cli/cmd-login.test.ts | 12 ++++++------ test/cli/cmd-remove.test.ts | 20 ++++++++++---------- test/cli/cmd-search.test.ts | 8 ++++---- 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/test/cli/cmd-deps.test.ts b/test/cli/cmd-deps.test.ts index a71ab676..af16ddae 100644 --- a/test/cli/cmd-deps.test.ts +++ b/test/cli/cmd-deps.test.ts @@ -13,7 +13,7 @@ import { mockService } from "../services/service.mock"; import { VersionNotFoundError } from "../../src/domain/packument"; import { noopLogger } from "../../src/logging"; import { GenericIOError } from "../../src/io/common-errors"; -import {ResultCodes} from "../../src/cli/result-codes"; +import { ResultCodes } from "../../src/cli/result-codes"; const somePackage = makeDomainName("com.some.package"); const otherPackage = makeDomainName("com.other.package"); @@ -60,9 +60,9 @@ describe("cmd-deps", () => { const { depsCmd, parseEnv } = makeDependencies(); parseEnv.mockResolvedValue(Err(expected)); - const result = await depsCmd(somePackage, { _global: {} }); + const resultCode = await depsCmd(somePackage, { _global: {} }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should fail if package-reference has url-version", async () => { diff --git a/test/cli/cmd-login.test.ts b/test/cli/cmd-login.test.ts index 3515cbc7..916264a2 100644 --- a/test/cli/cmd-login.test.ts +++ b/test/cli/cmd-login.test.ts @@ -47,9 +47,9 @@ describe("cmd-login", () => { const { loginCmd, parseEnv } = makeDependencies(); parseEnv.mockResolvedValue(Err(expected)); - const result = await loginCmd({ _global: {} }); + const resultCode = await loginCmd({ _global: {} }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); // TODO: Add tests for prompting logic @@ -59,14 +59,14 @@ describe("cmd-login", () => { const { loginCmd, getUpmConfigPath } = makeDependencies(); getUpmConfigPath.mockReturnValue(Err(expected).toAsyncResult()); - const result = await loginCmd({ + const resultCode = await loginCmd({ username: exampleUser, password: examplePassword, email: exampleEmail, _global: { registry: exampleRegistryUrl }, }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should fail if login failed", async () => { @@ -74,14 +74,14 @@ describe("cmd-login", () => { const { loginCmd, login } = makeDependencies(); login.mockReturnValue(Err(expected).toAsyncResult()); - const result = await loginCmd({ + const resultCode = await loginCmd({ username: exampleUser, password: examplePassword, email: exampleEmail, _global: { registry: exampleRegistryUrl }, }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if unauthorized", async () => { diff --git a/test/cli/cmd-remove.test.ts b/test/cli/cmd-remove.test.ts index 7a24e215..fbfa234a 100644 --- a/test/cli/cmd-remove.test.ts +++ b/test/cli/cmd-remove.test.ts @@ -62,18 +62,18 @@ describe("cmd-remove", () => { const { removeCmd, parseEnv } = makeDependencies(); parseEnv.mockResolvedValue(Err(expected)); - const result = await removeCmd(somePackage, { _global: {} }); + const resultCode = await removeCmd(somePackage, { _global: {} }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should fail if manifest could not be loaded", async () => { const { removeCmd, loadProjectManifest } = makeDependencies(); mockProjectManifest(loadProjectManifest, null); - const result = await removeCmd(somePackage, { _global: {} }); + const resultCode = await removeCmd(somePackage, { _global: {} }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if manifest could not be loaded", async () => { @@ -88,12 +88,12 @@ describe("cmd-remove", () => { it("should fail if package version was specified", async () => { const { removeCmd } = makeDependencies(); - const result = await removeCmd( + const resultCode = await removeCmd( makePackageReference(somePackage, makeSemanticVersion("1.0.0")), { _global: {} } ); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if package version was specified", async () => { @@ -113,9 +113,9 @@ describe("cmd-remove", () => { it("should fail if package is not in manifest", async () => { const { removeCmd } = makeDependencies(); - const result = await removeCmd(otherPackage, { _global: {} }); + const resultCode = await removeCmd(otherPackage, { _global: {} }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if package is not in manifest", async () => { @@ -182,9 +182,9 @@ describe("cmd-remove", () => { const { removeCmd, writeProjectManifest } = makeDependencies(); mockProjectManifestWriteResult(writeProjectManifest, expected); - const result = await removeCmd(somePackage, { _global: {} }); + const resultCode = await removeCmd(somePackage, { _global: {} }); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if manifest could not be saved", async () => { diff --git a/test/cli/cmd-search.test.ts b/test/cli/cmd-search.test.ts index 7b9a1526..77e253e3 100644 --- a/test/cli/cmd-search.test.ts +++ b/test/cli/cmd-search.test.ts @@ -66,9 +66,9 @@ describe("cmd-search", () => { it("should be ok if no network error occurred", async () => { const { searchCmd } = makeDependencies(); - const result = await searchCmd("pkg-not-exist", options); + const resultCode = await searchCmd("pkg-not-exist", options); - expect(result).toEqual(ResultCodes.Ok); + expect(resultCode).toEqual(ResultCodes.Ok); }); it("should notify of unknown packument", async () => { @@ -88,9 +88,9 @@ describe("cmd-search", () => { const { searchCmd, searchPackages } = makeDependencies(); searchPackages.mockReturnValue(Err(expected).toAsyncResult()); - const result = await searchCmd("package-a", options); + const resultCode = await searchCmd("package-a", options); - expect(result).toEqual(ResultCodes.Error); + expect(resultCode).toEqual(ResultCodes.Error); }); it("should notify if packuments could not be searched", async () => { From e86d44cdc10aa932f1d3748e0755f7afdd2d7a40 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 18:54:16 +0200 Subject: [PATCH 09/13] exception to log --- src/cli/cmd-deps.ts | 7 ++++--- test/cli/cmd-deps.test.ts | 22 ++++++++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/cli/cmd-deps.ts b/src/cli/cmd-deps.ts index 97975588..21cfed0c 100644 --- a/src/cli/cmd-deps.ts +++ b/src/cli/cmd-deps.ts @@ -62,9 +62,10 @@ export function makeDepsCmd( const [name, version] = splitPackageReference(pkg); - if (version !== undefined && isPackageUrl(version)) - // TODO: Convert to result - throw new Error("Cannot get dependencies for url-version"); + if (version !== undefined && isPackageUrl(version)) { + log.error("", "cannot get dependencies for url-version"); + return ResultCodes.Error; + } const deep = options.deep || false; debugLog(`fetch: ${makePackageReference(name, version)}, deep=${deep}`); diff --git a/test/cli/cmd-deps.test.ts b/test/cli/cmd-deps.test.ts index af16ddae..c3b99853 100644 --- a/test/cli/cmd-deps.test.ts +++ b/test/cli/cmd-deps.test.ts @@ -68,16 +68,30 @@ describe("cmd-deps", () => { it("should fail if package-reference has url-version", async () => { const { depsCmd } = makeDependencies(); - const operation = depsCmd( + const resultCode = await depsCmd( makePackageReference(somePackage, "https://some.registry.com"), { _global: {}, } ); - await expect(operation).rejects.toMatchObject({ - message: "Cannot get dependencies for url-version", - }); + expect(resultCode).toEqual(ResultCodes.Error); + }); + + it("should notify if package-reference has url-version", async () => { + const { depsCmd, log } = makeDependencies(); + + await depsCmd( + makePackageReference(somePackage, "https://some.registry.com"), + { + _global: {}, + } + ); + + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("url-version") + ); }); it("should log valid dependencies", async () => { From 9c1c4e1c976dd80ee68b649a1f54da022c7682c1 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 19:15:53 +0200 Subject: [PATCH 10/13] use jest assertion --- test/cli/cmd-add.test.ts | 32 ++++++++++++------------ test/cli/cmd-deps.test.ts | 6 ++--- test/cli/cmd-remove.test.ts | 12 ++++----- test/cli/cmd-search.test.ts | 2 +- test/cli/cmd-view.test.ts | 4 +-- test/cli/log.mock.ts | 43 +-------------------------------- test/services/parse-env.test.ts | 2 +- test/types/jest.d.ts | 16 ------------ 8 files changed, 30 insertions(+), 87 deletions(-) diff --git a/test/cli/cmd-add.test.ts b/test/cli/cmd-add.test.ts index 23707bbf..3abd9cc3 100644 --- a/test/cli/cmd-add.test.ts +++ b/test/cli/cmd-add.test.ts @@ -188,7 +188,7 @@ describe("cmd-add", () => { await addCmd(somePackage, { _global: {} }); - expect(log.error).toHaveLogLike( + expect(log.error).toHaveBeenCalledWith( expect.any(String), expect.stringContaining("could not load project manifest") ); @@ -209,7 +209,7 @@ describe("cmd-add", () => { await addCmd(somePackage, { _global: {} }); - expect(log.error).toHaveLogLike( + expect(log.error).toHaveBeenCalledWith( "404", expect.stringContaining("not found") ); @@ -235,7 +235,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "404", expect.stringContaining("is not a valid choice") ); @@ -249,7 +249,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "editor.version", expect.stringContaining("unknown") ); @@ -266,7 +266,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "package.unity", expect.stringContaining("not valid") ); @@ -283,7 +283,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "suggest", expect.stringContaining("run with option -f") ); @@ -329,7 +329,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "editor.version", expect.stringContaining("requires") ); @@ -346,7 +346,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "suggest", expect.stringContaining("run with option -f") ); @@ -400,7 +400,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "404", expect.stringContaining("is not a valid choice") ); @@ -440,7 +440,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "suggest", expect.stringContaining("manually") ); @@ -465,7 +465,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.error).toHaveLogLike( + expect(log.error).toHaveBeenCalledWith( "missing dependencies", expect.stringContaining("run with option -f") ); @@ -538,7 +538,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "manifest", expect.stringContaining("added") ); @@ -579,7 +579,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "manifest", expect.stringContaining("modified") ); @@ -598,7 +598,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "manifest", expect.stringContaining("existed") ); @@ -680,7 +680,7 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveLogLike("", expect.stringContaining("open Unity")); + expect(log.notice).toHaveBeenCalledWith("", expect.stringContaining("open Unity")); }); it("should fail if manifest could not be saved", async () => { @@ -699,6 +699,6 @@ describe("cmd-add", () => { await addCmd(somePackage, { _global: {} }); - expect(log.error).toHaveLogLike("manifest", expect.stringContaining("")); + expect(log.error).toHaveBeenCalledWith("manifest", expect.stringContaining("")); }); }); diff --git a/test/cli/cmd-deps.test.ts b/test/cli/cmd-deps.test.ts index c3b99853..77a4fc6f 100644 --- a/test/cli/cmd-deps.test.ts +++ b/test/cli/cmd-deps.test.ts @@ -101,7 +101,7 @@ describe("cmd-deps", () => { _global: {}, }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "dependency", expect.stringContaining(otherPackage) ); @@ -126,7 +126,7 @@ describe("cmd-deps", () => { _global: {}, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "missing dependency", expect.stringContaining(otherPackage) ); @@ -151,7 +151,7 @@ describe("cmd-deps", () => { _global: {}, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "missing dependency version", expect.stringContaining(otherPackage) ); diff --git a/test/cli/cmd-remove.test.ts b/test/cli/cmd-remove.test.ts index fbfa234a..b4f03bb3 100644 --- a/test/cli/cmd-remove.test.ts +++ b/test/cli/cmd-remove.test.ts @@ -82,7 +82,7 @@ describe("cmd-remove", () => { await removeCmd(somePackage, { _global: {} }); - expect(log.error).toHaveLogLike("manifest", expect.any(String)); + expect(log.error).toHaveBeenCalledWith("manifest", expect.any(String)); }); it("should fail if package version was specified", async () => { @@ -104,7 +104,7 @@ describe("cmd-remove", () => { { _global: {} } ); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "", expect.stringContaining("please do not specify") ); @@ -123,7 +123,7 @@ describe("cmd-remove", () => { await removeCmd(otherPackage, { _global: {} }); - expect(log.error).toHaveLogLike( + expect(log.error).toHaveBeenCalledWith( "404", expect.stringContaining("not found") ); @@ -134,7 +134,7 @@ describe("cmd-remove", () => { await removeCmd(somePackage, { _global: {} }); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "manifest", expect.stringContaining("removed") ); @@ -193,7 +193,7 @@ describe("cmd-remove", () => { await removeCmd(somePackage, { _global: {} }); - expect(log.error).toHaveLogLike("manifest", expect.stringContaining("")); + expect(log.error).toHaveBeenCalledWith("manifest", expect.stringContaining("")); }); it("should suggest to open Unity after save", async () => { @@ -201,6 +201,6 @@ describe("cmd-remove", () => { await removeCmd(somePackage, { _global: {} }); - expect(log.notice).toHaveLogLike("", expect.stringContaining("open Unity")); + expect(log.notice).toHaveBeenCalledWith("", expect.stringContaining("open Unity")); }); }); diff --git a/test/cli/cmd-search.test.ts b/test/cli/cmd-search.test.ts index 77e253e3..0bb2cf2f 100644 --- a/test/cli/cmd-search.test.ts +++ b/test/cli/cmd-search.test.ts @@ -77,7 +77,7 @@ describe("cmd-search", () => { await searchCmd("pkg-not-exist", options); - expect(log.notice).toHaveLogLike( + expect(log.notice).toHaveBeenCalledWith( "", expect.stringContaining("No matches found") ); diff --git a/test/cli/cmd-view.test.ts b/test/cli/cmd-view.test.ts index 6992d10d..ea553610 100644 --- a/test/cli/cmd-view.test.ts +++ b/test/cli/cmd-view.test.ts @@ -96,7 +96,7 @@ describe("cmd-view", () => { { _global: {} } ); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "", expect.stringContaining("please do not specify") ); @@ -127,7 +127,7 @@ describe("cmd-view", () => { await viewCmd(somePackage, { _global: {} }); - expect(log.error).toHaveLogLike( + expect(log.error).toHaveBeenCalledWith( "404", expect.stringContaining("not found") ); diff --git a/test/cli/log.mock.ts b/test/cli/log.mock.ts index afbfa5f2..9f864494 100644 --- a/test/cli/log.mock.ts +++ b/test/cli/log.mock.ts @@ -1,45 +1,4 @@ -import { Logger, LogLevels } from "npmlog"; -import AsymmetricMatcher = jest.AsymmetricMatcher; - -type LogSpy = jest.MockedFunctionDeep; - -expect.extend({ - toHaveLogLike( - spy: LogSpy, - prefix: string | AsymmetricMatcher, - expected: string | AsymmetricMatcher, - count: number = 1 - ) { - const stringMatches = (s: string, expected: string | AsymmetricMatcher) => - typeof expected === "string" - ? s === expected - : expected.asymmetricMatch(s); - - const calls = spy.mock.calls; - const callsWithPrefix = calls.filter(([actualPrefix]) => - stringMatches(actualPrefix, prefix) - ); - const matchingCalls = callsWithPrefix.filter(([, actualMessage]) => - stringMatches(actualMessage, expected) - ); - const hasMatch = matchingCalls.length >= count; - return { - pass: hasMatch, - message: () => `Logs failed expectation - Criteria: - Prefix: "${prefix}" - Message: "${expected}" - Min-count: ${count} - Issue: ${ - callsWithPrefix.length === 0 - ? "No logs had the correct prefix" - : matchingCalls.length === 0 - ? "At least one log had the correct prefix, but none had a matching message" - : `There were logs matching the criteria, but not enough (${matchingCalls.length})` - }`, - }; - }, -}); +import { Logger } from "npmlog"; /** * Creates mock logger. diff --git a/test/services/parse-env.test.ts b/test/services/parse-env.test.ts index c7ffa7de..171859ab 100644 --- a/test/services/parse-env.test.ts +++ b/test/services/parse-env.test.ts @@ -370,7 +370,7 @@ describe("env", () => { }, }); - expect(log.warn).toHaveLogLike( + expect(log.warn).toHaveBeenCalledWith( "env.auth", expect.stringContaining("failed to parse auth info") ); diff --git a/test/types/jest.d.ts b/test/types/jest.d.ts index 2d5bd241..e8d6b34d 100644 --- a/test/types/jest.d.ts +++ b/test/types/jest.d.ts @@ -19,22 +19,6 @@ declare global { toBeOk(valueAsserter?: (value: T) => void): R; toBeError(errorAsserter?: (error: T) => void): R; - - // Log - - /** - * Tests if a specific log was made to this spy. - * @param prefix The prefix. This is matched exactly. - * @param expected The expected value. Either an exact string that a log - * needs to match or a matcher. - * @param count The minimum number of times a message matching the given criteria - * should have been logged. Defaults to 1 if omitted. - */ - toHaveLogLike( - prefix: string | AsymmetricMatcher, - expected: string | AsymmetricMatcher, - count?: number - ): R; } } } From 61f9cd8e5647ca9b6082f23887aee5a98e825396 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 20:07:07 +0200 Subject: [PATCH 11/13] fix missing error class props --- src/common-errors.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/common-errors.ts b/src/common-errors.ts index 4c040cb2..c1897cc6 100644 --- a/src/common-errors.ts +++ b/src/common-errors.ts @@ -5,6 +5,8 @@ import { EditorVersion } from "./domain/editor-version"; * Error for when the packument was not found. */ export class PackumentNotFoundError extends CustomError { + // noinspection JSUnusedLocalSymbols + private readonly _class = "PackumentNotFoundError"; constructor() { super("A packument was not found."); } @@ -16,6 +18,8 @@ export class PackumentNotFoundError extends CustomError { * "com.my-package@1.2.3". */ export class PackageWithVersionError extends CustomError { + // noinspection JSUnusedLocalSymbols + private readonly _class = "PackageWithVersionError"; constructor() { super( "A package-reference including a version was specified when only a name was expected." @@ -27,6 +31,8 @@ export class PackageWithVersionError extends CustomError { * Error for when a file could not be parsed into a specific target type. */ export class FileParseError extends CustomError { + // noinspection JSUnusedLocalSymbols + private readonly _class = "FileParseError"; constructor( /** * The path to the file that could not be parsed. From dc30e3e3b4587e7ce0e6e2acc11c7caafd943531 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 20:19:18 +0200 Subject: [PATCH 12/13] better network errors --- src/cli/cmd-login.ts | 9 ++--- src/cli/error-logging.ts | 3 +- src/cli/index.ts | 6 +-- src/io/all-packuments-io.ts | 26 +++++++++++-- src/io/common-errors.ts | 18 +++++++++ src/io/npm-search.ts | 24 ++++++++++-- src/io/packument-io.ts | 18 +++++++-- src/packument-version-resolving.ts | 7 ++-- src/services/dependency-resolving.ts | 22 ++++++++--- src/services/npm-login.ts | 39 +++++++------------ src/services/search-packages.ts | 16 ++++++-- test/cli/cmd-login.test.ts | 24 +++--------- test/cli/cmd-search.test.ts | 6 +-- test/cli/cmd-view.test.ts | 8 ++-- test/io/all-packuments-io.test.ts | 29 ++++++++++++-- test/io/npm-search.test.ts | 29 ++++++++++++-- test/services/login.test.ts | 12 +++--- test/services/npm-login.test.ts | 9 +++-- .../services/resolve-remote-packument.test.ts | 4 +- test/services/search-packages.test.ts | 24 ++++++++---- 20 files changed, 223 insertions(+), 110 deletions(-) diff --git a/src/cli/cmd-login.ts b/src/cli/cmd-login.ts index 7d4a3749..dd5b0156 100644 --- a/src/cli/cmd-login.ts +++ b/src/cli/cmd-login.ts @@ -1,4 +1,3 @@ -import { AuthenticationError } from "../services/npm-login"; import { GetUpmConfigPath } from "../io/upm-config-io"; import { ParseEnvService } from "../services/parse-env"; import { coerceRegistryUrl } from "../domain/registry-url"; @@ -13,6 +12,7 @@ import { Logger } from "npmlog"; import { LoginService } from "../services/login"; import { logEnvParseError } from "./error-logging"; import { ResultCodes } from "./result-codes"; +import { RegistryAuthenticationError } from "../io/common-errors"; /** * Options for logging in a user. These come from the CLI. @@ -88,11 +88,8 @@ export function makeLoginCmd( if (loginResult.isErr()) { const loginError = loginResult.error; - if (loginError instanceof AuthenticationError) { - if (loginError.status === 401) - log.warn("401", "Incorrect username or password"); - else log.error(loginError.status.toString(), loginError.message); - } + if (loginError instanceof RegistryAuthenticationError) + log.warn("401", "Incorrect username or password"); // TODO: Log all errors return ResultCodes.Error; diff --git a/src/cli/error-logging.ts b/src/cli/error-logging.ts index ede5a6d5..249ab6ff 100644 --- a/src/cli/error-logging.ts +++ b/src/cli/error-logging.ts @@ -14,6 +14,7 @@ import { RequiredEnvMissingError } from "../io/upm-config-io"; import { FileMissingError, GenericIOError } from "../io/common-errors"; import { StringFormatError } from "../utils/string-parsing"; import { DetermineEditorVersionError } from "../services/determine-editor-version"; +import { ResolveRemotePackumentVersionError } from "../services/resolve-remote-packument-version"; /** * Logs a {@link ManifestLoadError} to the console. @@ -50,7 +51,7 @@ export function logManifestSaveError(log: Logger, error: ManifestWriteError) { export function logPackumentResolveError( log: Logger, packageName: DomainName, - error: PackumentVersionResolveError + error: ResolveRemotePackumentVersionError ) { if (error instanceof PackumentNotFoundError) log.error("404", `package not found: ${packageName}`); diff --git a/src/cli/index.ts b/src/cli/index.ts index 6d9eb87d..a477453a 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -73,8 +73,8 @@ const loadNpmrc = makeNpmrcLoader(readFile); const saveNpmrc = makeNpmrcSaver(writeFile); const loadProjectVersion = makeProjectVersionLoader(readFile); const fetchPackument = makePackumentFetcher(regClient); -const fetchAllPackuments = makeAllPackumentsFetcher(); -const searchRegistry = makeRegistrySearcher(); +const fetchAllPackuments = makeAllPackumentsFetcher(debugLog); +const searchRegistry = makeRegistrySearcher(debugLog); const resolveRemotePackument = makeRemotePackumentResolver(fetchPackument); const parseEnv = makeParseEnvService( @@ -85,7 +85,7 @@ const parseEnv = makeParseEnvService( ); const determineEditorVersion = makeEditorVersionDeterminer(loadProjectVersion); const authNpmrc = makeAuthNpmrcService(findNpmrcPath, loadNpmrc, saveNpmrc); -const npmLogin = makeNpmLoginService(regClient); +const npmLogin = makeNpmLoginService(regClient, debugLog); const resolveRemovePackumentVersion = makeResolveRemotePackumentVersionService(fetchPackument); const resolveLatestVersion = makeResolveLatestVersionService(fetchPackument); diff --git a/src/io/all-packuments-io.ts b/src/io/all-packuments-io.ts index 8575e6e4..a730132b 100644 --- a/src/io/all-packuments-io.ts +++ b/src/io/all-packuments-io.ts @@ -4,7 +4,11 @@ import npmFetch from "npm-registry-fetch"; import { assertIsHttpError } from "../utils/error-type-guards"; import { getNpmFetchOptions, SearchedPackument } from "./npm-search"; import { DomainName } from "../domain/domain-name"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "./common-errors"; +import { DebugLog } from "../logging"; /** * The result of querying the /-/all endpoint. @@ -14,18 +18,27 @@ export type AllPackuments = Readonly<{ [name: DomainName]: SearchedPackument; }>; +/** + * Error for when the request to get all packuments failed. + */ +export type FetchAllPackumentsError = + | GenericNetworkError + | RegistryAuthenticationError; + /** * Function for getting fetching packuments from a npm registry. * @param registry The registry to get packuments for. */ export type FetchAllPackuments = ( registry: Registry -) => AsyncResult; +) => AsyncResult; /** * Makes a {@link FetchAllPackuments} function. */ -export function makeAllPackumentsFetcher(): FetchAllPackuments { +export function makeAllPackumentsFetcher( + debugLog: DebugLog +): FetchAllPackuments { return (registry) => { return new AsyncResult( npmFetch @@ -33,7 +46,12 @@ export function makeAllPackumentsFetcher(): FetchAllPackuments { .then((result) => Ok(result as AllPackuments)) .catch((error) => { assertIsHttpError(error); - return Err(error); + debugLog("A http request failed.", error); + return Err( + error.statusCode === 401 + ? new RegistryAuthenticationError() + : new GenericNetworkError() + ); }) ); }; diff --git a/src/io/common-errors.ts b/src/io/common-errors.ts index 11ed4a1b..c19a7f69 100644 --- a/src/io/common-errors.ts +++ b/src/io/common-errors.ts @@ -29,3 +29,21 @@ export class FileMissingError extends CustomError { super(); } } + +/** + * Generic error for when some non-specific network operation failed. + */ +export class GenericNetworkError extends CustomError { + private readonly _class = "GenericNetworkError"; +} + +/** + * Error for when authentication with a registry failed. + */ +export class RegistryAuthenticationError extends CustomError { + private readonly _class = "RegistryAuthenticationError"; + + constructor() { + super(); + } +} diff --git a/src/io/npm-search.ts b/src/io/npm-search.ts index 78a8dba9..81ef75c9 100644 --- a/src/io/npm-search.ts +++ b/src/io/npm-search.ts @@ -5,7 +5,11 @@ import { assertIsHttpError } from "../utils/error-type-guards"; import { UnityPackument } from "../domain/packument"; import { SemanticVersion } from "../domain/semantic-version"; import { Registry } from "../domain/registry"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "./common-errors"; +import { DebugLog } from "../logging"; /** * A type representing a searched packument. Instead of having all versions @@ -17,6 +21,13 @@ export type SearchedPackument = Readonly< } >; +/** + * Error which may occur when searching a npm registry. + */ +export type SearchRegistryError = + | RegistryAuthenticationError + | GenericNetworkError; + /** * Function for searching packuments on a registry. * @param registry The registry to search. @@ -25,7 +36,7 @@ export type SearchedPackument = Readonly< export type SearchRegistry = ( registry: Registry, keyword: string -) => AsyncResult, HttpErrorBase>; +) => AsyncResult, SearchRegistryError>; /** * Get npm fetch options. @@ -44,7 +55,7 @@ export const getNpmFetchOptions = function ( /** * Makes a {@link SearchRegistry} function. */ -export function makeRegistrySearcher(): SearchRegistry { +export function makeRegistrySearcher(debugLog: DebugLog): SearchRegistry { return (registry, keyword) => { return new AsyncResult( npmSearch(keyword, getNpmFetchOptions(registry)) @@ -52,7 +63,12 @@ export function makeRegistrySearcher(): SearchRegistry { .then((results) => Ok(results as SearchedPackument[])) .catch((error) => { assertIsHttpError(error); - return Err(error); + debugLog("A http request failed.", error); + return Err( + error.statusCode === 401 + ? new RegistryAuthenticationError() + : new GenericNetworkError() + ); }) ); }; diff --git a/src/io/packument-io.ts b/src/io/packument-io.ts index c8562fcf..18e29916 100644 --- a/src/io/packument-io.ts +++ b/src/io/packument-io.ts @@ -4,12 +4,17 @@ import { assertIsHttpError } from "../utils/error-type-guards"; import { Registry } from "../domain/registry"; import { DomainName } from "../domain/domain-name"; import { UnityPackument } from "../domain/packument"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "./common-errors"; /** * Error which may occur when fetching a packument from a remote registry. */ -export type FetchPackumentError = HttpErrorBase; +export type FetchPackumentError = + | GenericNetworkError + | RegistryAuthenticationError; /** * Function for fetching a packument from a registry. * @param registry The registry to fetch from. @@ -39,7 +44,14 @@ export function makePackumentFetcher( if (error !== null) { assertIsHttpError(error); if (error.statusCode === 404) resolve(Ok(null)); - else resolve(Err(error)); + else + resolve( + Err( + error.statusCode === 401 + ? new RegistryAuthenticationError() + : new GenericNetworkError() + ) + ); } else resolve(Ok(packument)); } ); diff --git a/src/packument-version-resolving.ts b/src/packument-version-resolving.ts index feb1eae2..71260ab9 100644 --- a/src/packument-version-resolving.ts +++ b/src/packument-version-resolving.ts @@ -12,6 +12,7 @@ import { PackumentCache, tryGetFromCache } from "./packument-cache"; import { RegistryUrl } from "./domain/registry-url"; import { Err, Result } from "ts-results-es"; import { PackumentNotFoundError } from "./common-errors"; +import { ResolveRemotePackumentVersionError } from "./services/resolve-remote-packument-version"; /** * A version-reference that is resolvable. @@ -79,9 +80,9 @@ export function tryResolveFromCache( * @returns The more fixable failure. */ export function pickMostFixable( - a: Err, - b: Err -): Err { + a: Err, + b: Err +): Err { // Anything is more fixable than packument-not-found if ( a.error instanceof PackumentNotFoundError && diff --git a/src/services/dependency-resolving.ts b/src/services/dependency-resolving.ts index d3f9abb6..730a2fb5 100644 --- a/src/services/dependency-resolving.ts +++ b/src/services/dependency-resolving.ts @@ -10,7 +10,10 @@ import { } from "../packument-version-resolving"; import { RegistryUrl } from "../domain/registry-url"; import { Registry } from "../domain/registry"; -import { ResolveRemotePackumentVersionService } from "./resolve-remote-packument-version"; +import { + ResolveRemotePackumentVersionError, + ResolveRemotePackumentVersionService, +} from "./resolve-remote-packument-version"; import { areArraysEqual } from "../utils/array-utils"; import { dependenciesOf } from "../domain/package-manifest"; import { @@ -19,8 +22,11 @@ import { } from "./resolve-latest-version"; import { Err, Ok, Result } from "ts-results-es"; import { PackumentNotFoundError } from "../common-errors"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { FetchPackumentError } from "../io/packument-io"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "../io/common-errors"; export type DependencyBase = { /** @@ -159,7 +165,7 @@ export function makeResolveDependenciesService( // Search all given registries. let resolveResult: Result< ResolvedPackumentVersion, - PackumentVersionResolveError | HttpErrorBase + ResolveRemotePackumentVersionError > = Err(new PackumentNotFoundError()); for (const source of sources) { const result = await tryResolveFromRegistry( @@ -177,13 +183,17 @@ export function makeResolveDependenciesService( } if (resolveResult.isErr()) { - if (resolveResult.error instanceof HttpErrorBase) - return resolveResult; + const error = resolveResult.error; + if ( + error instanceof GenericNetworkError || + error instanceof RegistryAuthenticationError + ) + return Err(error); depsInvalid.push({ name: entryName, self: isSelf, - reason: resolveResult.error, + reason: error, }); continue; } diff --git a/src/services/npm-login.ts b/src/services/npm-login.ts index 07f8b3bc..14f177e8 100644 --- a/src/services/npm-login.ts +++ b/src/services/npm-login.ts @@ -1,28 +1,16 @@ import RegClient from "another-npm-registry-client"; import { RegistryUrl } from "../domain/registry-url"; -import { CustomError } from "ts-custom-error"; import { AsyncResult, Err, Ok } from "ts-results-es"; - -/** - * Error for when authentication failed. - */ -export class AuthenticationError extends CustomError { - private readonly _class = "AuthenticationError"; - constructor( - /** - * The http-response code returned by the server. - */ - readonly status: number, - message: string - ) { - super(message); - } -} +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "../io/common-errors"; +import { DebugLog } from "../logging"; /** * Error which may occur when logging a user into a npm registry. */ -export type NpmLoginError = AuthenticationError; +export type NpmLoginError = GenericNetworkError | RegistryAuthenticationError; /** * A token authenticating a user. @@ -48,7 +36,8 @@ export type NpmLoginService = ( * Makes a new {@link NpmLoginService} function. */ export function makeNpmLoginService( - registryClient: RegClient.Instance + registryClient: RegClient.Instance, + debugLog: DebugLog ): NpmLoginService { return (registryUrl, username, email, password) => { return new AsyncResult( @@ -57,16 +46,16 @@ export function makeNpmLoginService( registryUrl, { auth: { username, email, password } }, (error, responseData, _, response) => { - if (response !== undefined && !responseData.ok) + if (response !== undefined && !responseData.ok) { + debugLog("A http request failed.", response); resolve( Err( - new AuthenticationError( - response.statusCode, - response.statusMessage - ) + response.statusCode === 401 + ? new RegistryAuthenticationError() + : new GenericNetworkError() ) ); - else if (responseData.ok) resolve(Ok(responseData.token)); + } else if (responseData.ok) resolve(Ok(responseData.token)); // TODO: Handle error } diff --git a/src/services/search-packages.ts b/src/services/search-packages.ts index e4189e46..38105507 100644 --- a/src/services/search-packages.ts +++ b/src/services/search-packages.ts @@ -1,13 +1,21 @@ import { Registry } from "../domain/registry"; import { AsyncResult } from "ts-results-es"; import { SearchedPackument, SearchRegistry } from "../io/npm-search"; -import { FetchAllPackuments } from "../io/all-packuments-io"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; +import { + FetchAllPackuments, + FetchAllPackumentsError, +} from "../io/all-packuments-io"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "../io/common-errors"; /** * Error which may occur when searching for packages. */ -export type SearchPackagesError = HttpErrorBase; +export type SearchPackagesError = + | RegistryAuthenticationError + | GenericNetworkError; /** * A function for searching packages in a registry. @@ -32,7 +40,7 @@ export function makePackagesSearcher( function searchInAll( registry: Registry, keyword: string - ): AsyncResult { + ): AsyncResult { return fetchAllPackuments(registry).map((allPackuments) => { // eslint-disable-next-line @typescript-eslint/no-unused-vars const { _updated, ...packumentEntries } = allPackuments; diff --git a/test/cli/cmd-login.test.ts b/test/cli/cmd-login.test.ts index 916264a2..9487cb55 100644 --- a/test/cli/cmd-login.test.ts +++ b/test/cli/cmd-login.test.ts @@ -10,8 +10,10 @@ import { LoginService } from "../../src/services/login"; import { makeMockLogger } from "./log.mock"; import { exampleRegistryUrl } from "../domain/data-registry"; import { unityRegistryUrl } from "../../src/domain/registry-url"; -import { AuthenticationError } from "../../src/services/npm-login"; -import { GenericIOError } from "../../src/io/common-errors"; +import { + GenericIOError, + RegistryAuthenticationError, +} from "../../src/io/common-errors"; import { ResultCodes } from "../../src/cli/result-codes"; const defaultEnv = { @@ -87,7 +89,7 @@ describe("cmd-login", () => { it("should notify if unauthorized", async () => { const { loginCmd, login, log } = makeDependencies(); login.mockReturnValue( - Err(new AuthenticationError(401, "oof")).toAsyncResult() + Err(new RegistryAuthenticationError()).toAsyncResult() ); await loginCmd({ @@ -103,22 +105,6 @@ describe("cmd-login", () => { ); }); - it("should notify of other login errors", async () => { - const { loginCmd, login, log } = makeDependencies(); - login.mockReturnValue( - Err(new AuthenticationError(500, "oof")).toAsyncResult() - ); - - await loginCmd({ - username: exampleUser, - password: examplePassword, - email: exampleEmail, - _global: { registry: exampleRegistryUrl }, - }); - - expect(log.error).toHaveBeenCalledWith("500", "oof"); - }); - it("should notify of success", async () => { const { loginCmd, log } = makeDependencies(); diff --git a/test/cli/cmd-search.test.ts b/test/cli/cmd-search.test.ts index 0bb2cf2f..c0855fe4 100644 --- a/test/cli/cmd-search.test.ts +++ b/test/cli/cmd-search.test.ts @@ -8,9 +8,9 @@ import { Err, Ok } from "ts-results-es"; import { Env, ParseEnvService } from "../../src/services/parse-env"; import { mockService } from "../services/service.mock"; import { SearchPackages } from "../../src/services/search-packages"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { noopLogger } from "../../src/logging"; import { ResultCodes } from "../../src/cli/result-codes"; +import { GenericNetworkError } from "../../src/io/common-errors"; const exampleSearchResult: SearchedPackument = { name: makeDomainName("com.example.package-a"), @@ -84,7 +84,7 @@ describe("cmd-search", () => { }); it("should fail if packuments could not be searched", async () => { - const expected = { statusCode: 500 } as HttpErrorBase; + const expected = new GenericNetworkError(); const { searchCmd, searchPackages } = makeDependencies(); searchPackages.mockReturnValue(Err(expected).toAsyncResult()); @@ -94,7 +94,7 @@ describe("cmd-search", () => { }); it("should notify if packuments could not be searched", async () => { - const expected = { statusCode: 500 } as HttpErrorBase; + const expected = new GenericNetworkError(); const { searchCmd, searchPackages, log } = makeDependencies(); searchPackages.mockReturnValue(Err(expected).toAsyncResult()); diff --git a/test/cli/cmd-view.test.ts b/test/cli/cmd-view.test.ts index ea553610..5c1479ac 100644 --- a/test/cli/cmd-view.test.ts +++ b/test/cli/cmd-view.test.ts @@ -10,8 +10,10 @@ import { makeMockLogger } from "./log.mock"; import { buildPackument } from "../domain/data-packument"; import { mockService } from "../services/service.mock"; import { ResolveRemotePackument } from "../../src/services/resolve-remote-packument"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; -import { GenericIOError } from "../../src/io/common-errors"; +import { + GenericIOError, + GenericNetworkError, +} from "../../src/io/common-errors"; import { ResultCodes } from "../../src/cli/result-codes"; const somePackage = makeDomainName("com.some.package"); @@ -112,7 +114,7 @@ describe("cmd-view", () => { }); it("should fail if package could not be resolved", async () => { - const expected = { statusCode: 500 } as HttpErrorBase; + const expected = new GenericNetworkError(); const { viewCmd, resolveRemotePackument } = makeDependencies(); resolveRemotePackument.mockReturnValue(Err(expected).toAsyncResult()); diff --git a/test/io/all-packuments-io.test.ts b/test/io/all-packuments-io.test.ts index b32b7094..e06506c9 100644 --- a/test/io/all-packuments-io.test.ts +++ b/test/io/all-packuments-io.test.ts @@ -3,6 +3,11 @@ import { makeAllPackumentsFetcher } from "../../src/io/all-packuments-io"; import { Registry } from "../../src/domain/registry"; import { exampleRegistryUrl } from "../domain/data-registry"; import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; +import { noopLogger } from "../../src/logging"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "../../src/io/common-errors"; jest.mock("npm-registry-fetch"); @@ -12,12 +17,12 @@ const exampleRegistry: Registry = { }; function makeDependencies() { - const getAllPackuments = makeAllPackumentsFetcher(); + const getAllPackuments = makeAllPackumentsFetcher(noopLogger); return { getAllPackuments } as const; } describe("fetch all packuments", () => { - it("should fail on error response", async () => { + it("should fail on non-auth error response", async () => { const expected = { message: "Idk, it failed", name: "FakeError", @@ -28,7 +33,25 @@ describe("fetch all packuments", () => { const result = await getAllPackuments(exampleRegistry).promise; - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toBeError((actual) => + expect(actual).toBeInstanceOf(GenericNetworkError) + ); + }); + + it("should fail on auth error response", async () => { + const expected = { + message: "Idk, it failed", + name: "FakeError", + statusCode: 401, + } as HttpErrorBase; + jest.mocked(npmFetch.json).mockRejectedValue(expected); + const { getAllPackuments } = makeDependencies(); + + const result = await getAllPackuments(exampleRegistry).promise; + + expect(result).toBeError((actual) => + expect(actual).toBeInstanceOf(RegistryAuthenticationError) + ); }); it("should succeed on ok response", async () => { diff --git a/test/io/npm-search.test.ts b/test/io/npm-search.test.ts index 381c569f..e45fcce8 100644 --- a/test/io/npm-search.test.ts +++ b/test/io/npm-search.test.ts @@ -4,6 +4,11 @@ import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { makeRegistrySearcher } from "../../src/io/npm-search"; import { Registry } from "../../src/domain/registry"; import { exampleRegistryUrl } from "../domain/data-registry"; +import { noopLogger } from "../../src/logging"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "../../src/io/common-errors"; jest.mock("libnpmsearch"); @@ -13,12 +18,12 @@ const exampleRegistry: Registry = { }; function makeDependencies() { - const searchRegistry = makeRegistrySearcher(); + const searchRegistry = makeRegistrySearcher(noopLogger); return { searchRegistry } as const; } describe("npm search", () => { - it("should fail for error response", async () => { + it("should fail for non-auth error response", async () => { const expected = { message: "Idk, it failed", name: "FakeError", @@ -29,7 +34,25 @@ describe("npm search", () => { const result = await searchRegistry(exampleRegistry, "wow").promise; - expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + expect(result).toBeError((actual) => + expect(actual).toBeInstanceOf(GenericNetworkError) + ); + }); + + it("should fail for auth error response", async () => { + const expected = { + message: "Idk, it failed", + name: "FakeError", + statusCode: 401, + } as HttpErrorBase; + jest.mocked(npmSearch).mockRejectedValue(expected); + const { searchRegistry } = makeDependencies(); + + const result = await searchRegistry(exampleRegistry, "wow").promise; + + expect(result).toBeError((actual) => + expect(actual).toBeInstanceOf(RegistryAuthenticationError) + ); }); it("should succeed for ok response", async () => { diff --git a/test/services/login.test.ts b/test/services/login.test.ts index e3350386..85da47b1 100644 --- a/test/services/login.test.ts +++ b/test/services/login.test.ts @@ -1,15 +1,15 @@ import { makeLoginService } from "../../src/services/login"; import { mockService } from "./service.mock"; import { SaveAuthToUpmConfig } from "../../src/services/upm-auth"; -import { - AuthenticationError, - NpmLoginService, -} from "../../src/services/npm-login"; +import { NpmLoginService } from "../../src/services/npm-login"; import { AuthNpmrcService } from "../../src/services/npmrc-auth"; import { exampleRegistryUrl } from "../domain/data-registry"; import { Err, Ok } from "ts-results-es"; import { noopLogger } from "../../src/logging"; -import { GenericIOError } from "../../src/io/common-errors"; +import { + RegistryAuthenticationError, + GenericIOError, +} from "../../src/io/common-errors"; const exampleUser = "user"; const examplePassword = "pass"; @@ -84,7 +84,7 @@ describe("login", () => { describe("token auth", () => { it("should fail if npm login fails", async () => { - const expected = new AuthenticationError(401, "uh oh"); + const expected = new RegistryAuthenticationError(); const { login, npmLogin } = makeDependencies(); npmLogin.mockReturnValue(Err(expected).toAsyncResult()); diff --git a/test/services/npm-login.test.ts b/test/services/npm-login.test.ts index a3d66822..1c308b80 100644 --- a/test/services/npm-login.test.ts +++ b/test/services/npm-login.test.ts @@ -1,12 +1,13 @@ import "assert"; import { - AuthenticationError, makeNpmLoginService, } from "../../src/services/npm-login"; import RegClient from "another-npm-registry-client"; import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { exampleRegistryUrl } from "../domain/data-registry"; import { mockRegClientAddUserResult } from "./registry-client.mock"; +import {RegistryAuthenticationError} from "../../src/io/common-errors"; +import {noopLogger} from "../../src/logging"; function makeDependencies() { const registryClient: jest.Mocked = { @@ -14,7 +15,7 @@ function makeDependencies() { get: jest.fn(), }; - const addUser = makeNpmLoginService(registryClient); + const addUser = makeNpmLoginService(registryClient, noopLogger); return { addUser, registryClient } as const; } @@ -64,7 +65,7 @@ describe("npm-login service", () => { ).promise; expect(result).toBeError((error) => - expect(error).toBeInstanceOf(AuthenticationError) + expect(error).toBeInstanceOf(RegistryAuthenticationError) ); }); @@ -88,7 +89,7 @@ describe("npm-login service", () => { ).promise; expect(result).toBeError((error) => - expect(error).toBeInstanceOf(AuthenticationError) + expect(error).toBeInstanceOf(RegistryAuthenticationError) ); }); }); diff --git a/test/services/resolve-remote-packument.test.ts b/test/services/resolve-remote-packument.test.ts index 915403d2..369561c3 100644 --- a/test/services/resolve-remote-packument.test.ts +++ b/test/services/resolve-remote-packument.test.ts @@ -7,7 +7,7 @@ import { buildPackument } from "../domain/data-packument"; import { mockService } from "./service.mock"; import { FetchPackument } from "../../src/io/packument-io"; import { Err, Ok } from "ts-results-es"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; +import { GenericNetworkError } from "../../src/io/common-errors"; describe("resolve remove packument", () => { const exampleName = makeDomainName("com.some.package"); @@ -84,7 +84,7 @@ describe("resolve remove packument", () => { }); it("should fail if any packument fetch failed", async () => { - const expected = { statusCode: 500 } as HttpErrorBase; + const expected = new GenericNetworkError(); const { resolveRemotePackument, fetchPackument } = makeDependencies(); fetchPackument.mockReturnValue(Err(expected).toAsyncResult()); diff --git a/test/services/search-packages.test.ts b/test/services/search-packages.test.ts index 9c9b8371..be84b1d4 100644 --- a/test/services/search-packages.test.ts +++ b/test/services/search-packages.test.ts @@ -7,10 +7,10 @@ import { import { makePackagesSearcher } from "../../src/services/search-packages"; import { Registry } from "../../src/domain/registry"; import { exampleRegistryUrl } from "../domain/data-registry"; -import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { Err, Ok } from "ts-results-es"; import { makeDomainName } from "../../src/domain/domain-name"; import { makeSemanticVersion } from "../../src/domain/semantic-version"; +import { GenericNetworkError } from "../../src/io/common-errors"; describe("search packages", () => { const exampleRegistry: Registry = { @@ -20,8 +20,6 @@ describe("search packages", () => { const exampleKeyword = "package-a"; - const exampleHttpError = { statusCode: 500 } as HttpErrorBase; - const exampleSearchResult: SearchedPackument = { name: makeDomainName("com.example.package-a"), versions: { [makeSemanticVersion("1.0.0")]: "latest" }, @@ -73,7 +71,9 @@ describe("search packages", () => { it("should search using old search if search api is not available", async () => { const { searchPackages, searchRegistry } = makeDependencies(); - searchRegistry.mockReturnValue(Err(exampleHttpError).toAsyncResult()); + searchRegistry.mockReturnValue( + Err(new GenericNetworkError()).toAsyncResult() + ); const result = await searchPackages(exampleRegistry, exampleKeyword) .promise; @@ -85,7 +85,9 @@ describe("search packages", () => { it("should notify of using old search", async () => { const { searchPackages, searchRegistry } = makeDependencies(); - searchRegistry.mockReturnValue(Err(exampleHttpError).toAsyncResult()); + searchRegistry.mockReturnValue( + Err(new GenericNetworkError()).toAsyncResult() + ); const fallback = jest.fn(); await searchPackages(exampleRegistry, exampleKeyword, fallback).promise; @@ -95,7 +97,9 @@ describe("search packages", () => { it("should not find packages not matching the keyword using old search", async () => { const { searchPackages, searchRegistry } = makeDependencies(); - searchRegistry.mockReturnValue(Err(exampleHttpError).toAsyncResult()); + searchRegistry.mockReturnValue( + Err(new GenericNetworkError()).toAsyncResult() + ); const result = await searchPackages(exampleRegistry, "some other keyword") .promise; @@ -106,8 +110,12 @@ describe("search packages", () => { it("should fail if both search strategies fail", async () => { const { searchPackages, searchRegistry, fetchAllPackument } = makeDependencies(); - searchRegistry.mockReturnValue(Err(exampleHttpError).toAsyncResult()); - fetchAllPackument.mockReturnValue(Err(exampleHttpError).toAsyncResult()); + searchRegistry.mockReturnValue( + Err(new GenericNetworkError()).toAsyncResult() + ); + fetchAllPackument.mockReturnValue( + Err(new GenericNetworkError()).toAsyncResult() + ); const result = await searchPackages(exampleRegistry, exampleKeyword) .promise; From 9fa5f0bd69fa65ef91294b7cb682dd3bf2a28a41 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 28 May 2024 20:43:44 +0200 Subject: [PATCH 13/13] add more error messages --- src/cli/cmd-add.ts | 40 ++-- src/cli/cmd-deps.ts | 13 +- src/cli/cmd-login.ts | 4 +- src/cli/cmd-remove.ts | 16 +- src/cli/cmd-search.ts | 4 +- src/cli/cmd-view.ts | 4 +- src/cli/error-logging.ts | 322 ++++++++++++++++++++++---------- test/cli/cmd-add.test.ts | 30 +-- test/cli/cmd-remove.test.ts | 15 +- test/services/npm-login.test.ts | 8 +- 10 files changed, 310 insertions(+), 146 deletions(-) diff --git a/src/cli/cmd-add.ts b/src/cli/cmd-add.ts index e8368079..3b0e08d2 100644 --- a/src/cli/cmd-add.ts +++ b/src/cli/cmd-add.ts @@ -34,13 +34,6 @@ import { SemanticVersion } from "../domain/semantic-version"; import { areArraysEqual } from "../utils/array-utils"; import { Err, Ok, Result } from "ts-results-es"; import { CustomError } from "ts-custom-error"; -import { - logDetermineEditorError, - logEnvParseError, - logManifestLoadError, - logManifestSaveError, - logPackumentResolveError, -} from "./error-logging"; import { DependencyResolveError, ResolveDependenciesService, @@ -55,6 +48,13 @@ import { DebugLog } from "../logging"; import { DetermineEditorVersion } from "../services/determine-editor-version"; import { FetchPackumentError } from "../io/packument-io"; import { ResultCodes } from "./result-codes"; +import { + notifyEnvParsingFailed, + notifyManifestLoadFailed, + notifyManifestWriteFailed, + notifyProjectVersionLoadFailed, + notifyRemotePackumentVersionResolvingFailed, +} from "./error-logging"; export class InvalidPackumentDataError extends CustomError { private readonly _class = "InvalidPackumentDataError"; @@ -126,14 +126,14 @@ export function makeAddCmd( // parse env const envResult = await parseEnv(options); if (envResult.isErr()) { - logEnvParseError(log, envResult.error); + notifyEnvParsingFailed(log, envResult.error); return ResultCodes.Error; } const env = envResult.value; const editorVersionResult = await determineEditorVersion(env.cwd).promise; if (editorVersionResult.isErr()) { - logDetermineEditorError(log, editorVersionResult.error); + notifyProjectVersionLoadFailed(log, editorVersionResult.error); return ResultCodes.Error; } const editorVersion = editorVersionResult.value; @@ -177,7 +177,11 @@ export function makeAddCmd( } if (resolveResult.isErr()) { - logPackumentResolveError(log, name, resolveResult.error); + notifyRemotePackumentVersionResolvingFailed( + log, + name, + resolveResult.error + ); return resolveResult; } @@ -235,7 +239,11 @@ export function makeAddCmd( true ); if (resolveResult.isErr()) { - logPackumentResolveError(log, name, resolveResult.error); + notifyRemotePackumentVersionResolvingFailed( + log, + name, + resolveResult.error + ); return resolveResult; } const [depsValid, depsInvalid] = resolveResult.value; @@ -255,7 +263,11 @@ export function makeAddCmd( // print suggestion for depsInvalid let isAnyDependencyUnresolved = false; depsInvalid.forEach((depObj) => { - logPackumentResolveError(log, depObj.name, depObj.reason); + notifyRemotePackumentVersionResolvingFailed( + log, + depObj.name, + depObj.reason + ); // If the manifest already has the dependency than it does not // really matter that it was not resolved. @@ -332,7 +344,7 @@ export function makeAddCmd( // load manifest const loadResult = await loadProjectManifest(env.cwd).promise; if (loadResult.isErr()) { - logManifestLoadError(log, loadResult.error); + notifyManifestLoadFailed(log, loadResult.error); return ResultCodes.Error; } let manifest = loadResult.value; @@ -354,7 +366,7 @@ export function makeAddCmd( if (dirty) { const saveResult = await writeProjectManifest(env.cwd, manifest).promise; if (saveResult.isErr()) { - logManifestSaveError(log, saveResult.error); + notifyManifestWriteFailed(log); return ResultCodes.Error; } diff --git a/src/cli/cmd-deps.ts b/src/cli/cmd-deps.ts index 21cfed0c..7dbef5c3 100644 --- a/src/cli/cmd-deps.ts +++ b/src/cli/cmd-deps.ts @@ -12,9 +12,12 @@ import { ResolveDependenciesService } from "../services/dependency-resolving"; import { Logger } from "npmlog"; import { logValidDependency } from "./dependency-logging"; import { VersionNotFoundError } from "../domain/packument"; -import { logEnvParseError, logPackumentResolveError } from "./error-logging"; import { DebugLog } from "../logging"; import { ResultCodes } from "./result-codes"; +import { + notifyEnvParsingFailed, + notifyRemotePackumentVersionResolvingFailed, +} from "./error-logging"; export type DepsOptions = CmdOptions<{ deep?: boolean; @@ -55,7 +58,7 @@ export function makeDepsCmd( // parse env const envResult = await parseEnv(options); if (envResult.isErr()) { - logEnvParseError(log, envResult.error); + notifyEnvParsingFailed(log, envResult.error); return ResultCodes.Error; } const env = envResult.value; @@ -76,7 +79,11 @@ export function makeDepsCmd( deep ); if (resolveResult.isErr()) { - logPackumentResolveError(log, name, resolveResult.error); + notifyRemotePackumentVersionResolvingFailed( + log, + name, + resolveResult.error + ); return ResultCodes.Error; } diff --git a/src/cli/cmd-login.ts b/src/cli/cmd-login.ts index dd5b0156..604092e6 100644 --- a/src/cli/cmd-login.ts +++ b/src/cli/cmd-login.ts @@ -10,9 +10,9 @@ import { import { CmdOptions } from "./options"; import { Logger } from "npmlog"; import { LoginService } from "../services/login"; -import { logEnvParseError } from "./error-logging"; import { ResultCodes } from "./result-codes"; import { RegistryAuthenticationError } from "../io/common-errors"; +import { notifyEnvParsingFailed } from "./error-logging"; /** * Options for logging in a user. These come from the CLI. @@ -51,7 +51,7 @@ export function makeLoginCmd( // parse env const envResult = await parseEnv(options); if (envResult.isErr()) { - logEnvParseError(log, envResult.error); + notifyEnvParsingFailed(log, envResult.error); return ResultCodes.Error; } const env = envResult.value; diff --git a/src/cli/cmd-remove.ts b/src/cli/cmd-remove.ts index f682abcc..24765e4f 100644 --- a/src/cli/cmd-remove.ts +++ b/src/cli/cmd-remove.ts @@ -24,13 +24,13 @@ import { PackageWithVersionError, PackumentNotFoundError, } from "../common-errors"; -import { - logEnvParseError, - logManifestLoadError, - logManifestSaveError, -} from "./error-logging"; import { Logger } from "npmlog"; import { ResultCodes } from "./result-codes"; +import { + notifyEnvParsingFailed, + notifyManifestLoadFailed, + notifyManifestWriteFailed, +} from "./error-logging"; /** * The possible result codes with which the remove command can exit. @@ -70,7 +70,7 @@ export function makeRemoveCmd( // parse env const envResult = await parseEnv(options); if (envResult.isErr()) { - logEnvParseError(log, envResult.error); + notifyEnvParsingFailed(log, envResult.error); return ResultCodes.Error; } const env = envResult.value; @@ -110,7 +110,7 @@ export function makeRemoveCmd( // load manifest const manifestResult = await loadProjectManifest(env.cwd).promise; if (manifestResult.isErr()) { - logManifestLoadError(log, manifestResult.error); + notifyManifestLoadFailed(log, manifestResult.error); return ResultCodes.Error; } let manifest = manifestResult.value; @@ -125,7 +125,7 @@ export function makeRemoveCmd( // save manifest const saveResult = await writeProjectManifest(env.cwd, manifest).promise; if (saveResult.isErr()) { - logManifestSaveError(log, saveResult.error); + notifyManifestWriteFailed(log); return ResultCodes.Error; } diff --git a/src/cli/cmd-search.ts b/src/cli/cmd-search.ts index 97f55dc8..3cc3fd15 100644 --- a/src/cli/cmd-search.ts +++ b/src/cli/cmd-search.ts @@ -4,9 +4,9 @@ import { CmdOptions } from "./options"; import { formatAsTable } from "./output-formatting"; import { Logger } from "npmlog"; import { SearchPackages } from "../services/search-packages"; -import { logEnvParseError } from "./error-logging"; import { DebugLog } from "../logging"; import { ResultCodes } from "./result-codes"; +import { notifyEnvParsingFailed } from "./error-logging"; /** * The possible result codes with which the search command can exit. @@ -38,7 +38,7 @@ export function makeSearchCmd( // parse env const envResult = await parseEnv(options); if (envResult.isErr()) { - logEnvParseError(log, envResult.error); + notifyEnvParsingFailed(log, envResult.error); return ResultCodes.Error; } const env = envResult.value; diff --git a/src/cli/cmd-view.ts b/src/cli/cmd-view.ts index 4f06318e..ab7c6ddf 100644 --- a/src/cli/cmd-view.ts +++ b/src/cli/cmd-view.ts @@ -11,8 +11,8 @@ import { CmdOptions } from "./options"; import { recordKeys } from "../utils/record-utils"; import { Logger } from "npmlog"; import { ResolveRemotePackument } from "../services/resolve-remote-packument"; -import { logEnvParseError } from "./error-logging"; import { ResultCodes } from "./result-codes"; +import { notifyEnvParsingFailed } from "./error-logging"; export type ViewOptions = CmdOptions; @@ -112,7 +112,7 @@ export function makeViewCmd( // parse env const envResult = await parseEnv(options); if (envResult.isErr()) { - logEnvParseError(log, envResult.error); + notifyEnvParsingFailed(log, envResult.error); return ResultCodes.Error; } const env = envResult.value; diff --git a/src/cli/error-logging.ts b/src/cli/error-logging.ts index 249ab6ff..fb857c9c 100644 --- a/src/cli/error-logging.ts +++ b/src/cli/error-logging.ts @@ -1,110 +1,242 @@ -import { - ManifestLoadError, - ManifestWriteError, -} from "../io/project-manifest-io"; import { Logger } from "npmlog"; -import { PackumentVersionResolveError } from "../packument-version-resolving"; -import { FileParseError, PackumentNotFoundError } from "../common-errors"; -import { DomainName } from "../domain/domain-name"; -import { VersionNotFoundError } from "../domain/packument"; import { EnvParseError } from "../services/parse-env"; import { NoWslError } from "../io/wsl"; import { ChildProcessError } from "../utils/process"; import { RequiredEnvMissingError } from "../io/upm-config-io"; -import { FileMissingError, GenericIOError } from "../io/common-errors"; +import { + FileMissingError, + GenericIOError, + GenericNetworkError, + RegistryAuthenticationError, +} from "../io/common-errors"; import { StringFormatError } from "../utils/string-parsing"; -import { DetermineEditorVersionError } from "../services/determine-editor-version"; +import { ProjectVersionLoadError } from "../io/project-version-io"; +import { FileParseError, PackumentNotFoundError } from "../common-errors"; +import { DomainName } from "../domain/domain-name"; +import { NoVersionsError, VersionNotFoundError } from "../domain/packument"; +import { SemanticVersion } from "../domain/semantic-version"; +import { EOL } from "node:os"; import { ResolveRemotePackumentVersionError } from "../services/resolve-remote-packument-version"; +import { ManifestLoadError } from "../io/project-manifest-io"; + +export function suggestCheckingWorkingDirectory(log: Logger) { + log.notice("", "Are you in the correct working directory?"); +} + +export function notifyManifestMissing(log: Logger, filePath: string) { + log.error("", `Could not locate your project manifest at "${filePath}".`); + suggestCheckingWorkingDirectory(log); +} + +export function suggestFixErrorsInProjectManifest(log: Logger) { + log.notice( + "", + "Please fix the errors in your project manifest and try again." + ); +} + +export function notifySyntacticallyMalformedProjectManifest(log: Logger) { + log.error("", "Project manifest file contained json syntax errors."); + suggestFixErrorsInProjectManifest(log); +} + +export function notifySemanticallyMalformedProjectManifest(log: Logger) { + log.error( + "", + "Project manifest is valid json but was not of the correct shape." + ); + suggestFixErrorsInProjectManifest(log); +} + +export function notifyManifestLoadFailedBecauseIO(log: Logger) { + log.error( + "", + "Could not load project manifest because of a file-system error." + ); +} -/** - * Logs a {@link ManifestLoadError} to the console. - */ -export function logManifestLoadError(log: Logger, error: ManifestLoadError) { - const reason = - error instanceof FileMissingError - ? `it could not be found at "${error.path}"` - : error instanceof GenericIOError - ? "a file-system interaction failed" - : error instanceof StringFormatError - ? "the manifest file did not contain valid json" - : "the manifest file did not contain a valid project manifest"; - - const prefix = "manifest"; - const errorMessage = `could not load project manifest because ${reason}.`; - log.error(prefix, errorMessage); - - // TODO: Print fix suggestions -} - -/** - * Logs a {@link ManifestWriteError} to the console. - */ -export function logManifestSaveError(log: Logger, error: ManifestWriteError) { - const prefix = "manifest"; - log.error(prefix, "can not write manifest json file"); - log.error(prefix, error.message); -} - -/** - * Logs a {@link PackumentVersionResolveError} to the console. - */ -export function logPackumentResolveError( +export function notifyManifestLoadFailed( + log: Logger, + error: ManifestLoadError +) { + if (error instanceof FileMissingError) notifyManifestMissing(log, error.path); + else if (error instanceof StringFormatError) + notifySyntacticallyMalformedProjectManifest(log); + else if (error instanceof FileParseError) + notifySemanticallyMalformedProjectManifest(log); + else if (error instanceof GenericIOError) + notifyManifestLoadFailedBecauseIO(log); +} + +export function notifyManifestWriteFailed(log: Logger) { + log.error( + "", + "Could not save project manifest because of a file-system error." + ); +} + +export function notifyNotUsingWsl(log: Logger) { + log.error("", "No wsl detected."); + log.notice("", "Please make sure you are actually running openupm in wsl."); +} + +export function notifyChildProcessError(log: Logger) { + log.error("", "A child process encountered an error."); +} + +export function notifyMissingEnvForUpmConfigPath( + log: Logger, + variableNames: string[] +) { + const nameList = variableNames.map((name) => `"${name}"`).join(", "); + log.error( + "", + "Could not determine upm-config path because of missing home environment variables." + ); + log.notice( + "", + `Please make sure that you set one of the following environment variables: ${nameList}.` + ); +} + +export function notifySyntacticallyMalformedUpmConfig(log: Logger) { + log.error("", "Upm-config file contained toml syntax errors."); + log.notice("", "Please fix the errors in your upm-config and try again."); +} + +function notifyUpmConfigLoadFailedBecauseIO(log: Logger) { + log.error("", "Could not load upm-config because of a file-system error."); +} + +export function notifyEnvParsingFailed(log: Logger, error: EnvParseError) { + if (error instanceof NoWslError) notifyNotUsingWsl(log); + else if (error instanceof ChildProcessError) notifyChildProcessError(log); + else if (error instanceof RequiredEnvMissingError) + notifyMissingEnvForUpmConfigPath(log, error.keyNames); + else if (error instanceof GenericIOError) + notifyUpmConfigLoadFailedBecauseIO(log); + else if (error instanceof StringFormatError) + notifySyntacticallyMalformedUpmConfig(log); +} + +export function notifyProjectVersionMissing(log: Logger, filePath: string) { + log.error( + "", + `Could not locate your projects version file (ProjectVersion.txt) at "${filePath}".` + ); + suggestCheckingWorkingDirectory(log); +} + +export function suggestFixErrorsInProjectVersionFile(log: Logger) { + log.notice( + "", + "Please fix the errors in your project version file and try again." + ); +} + +export function notifySyntacticallyMalformedProjectVersion(log: Logger) { + log.error( + "", + "Project version file (ProjectVersion.txt) file contained yaml syntax errors." + ); + suggestFixErrorsInProjectVersionFile(log); +} + +export function notifySemanticallyMalformedProjectVersion(log: Logger) { + log.error( + "", + "Project version file (ProjectVersion.txt) file is valid yaml but was not of the correct shape." + ); + suggestFixErrorsInProjectVersionFile(log); +} + +export function notifyProjectVersionLoadFailed( + log: Logger, + error: ProjectVersionLoadError +) { + if (error instanceof FileMissingError) + notifyProjectVersionMissing(log, error.path); + else if (error instanceof GenericIOError) + log.error( + "", + "Could not load project version file (ProjectVersion.txt) because of a file-system error." + ); + else if (error instanceof StringFormatError) + notifySyntacticallyMalformedProjectVersion(log); + else if (error instanceof FileParseError) + notifySemanticallyMalformedProjectVersion(log); +} + +export function notifyPackumentNotFoundInAnyRegistry( + log: Logger, + packageName: DomainName +) { + log.error( + "", + `The package "${packageName}" was not found in any of the provided registries.` + ); + log.notice( + "", + "Please make sure you have spelled the name and registry urls correctly." + ); +} + +export function notifyNoVersions(log: Logger, packageName: DomainName) { + log.error("", `The package ${packageName} has no versions.`); +} + +export function notifyOfMissingVersion( + log: Logger, + packageName: DomainName, + requestedVersion: SemanticVersion, + availableVersions: ReadonlyArray +) { + const versionList = availableVersions + .map((version) => `\t- ${version}`) + .join(EOL); + + log.error( + "", + `The package "${packageName}" has no published version "${requestedVersion}".` + ); + log.notice("", `Maybe you meant one of the following:${EOL}${versionList}`); +} + +export function notifyRegistryCallFailedBecauseHttp(log: Logger) { + log.error( + "", + "Could not communicate with registry because of an http error." + ); +} + +export function notifyRegistryCallFailedBecauseUnauthorized(log: Logger) { + log.error( + "", + "An npm registry rejected your request, because you are unauthorized." + ); + log.notice( + "", + "Please make sure you are correctly authenticated for the registry." + ); +} + +export function notifyRemotePackumentVersionResolvingFailed( log: Logger, packageName: DomainName, error: ResolveRemotePackumentVersionError ) { if (error instanceof PackumentNotFoundError) - log.error("404", `package not found: ${packageName}`); - else if (error instanceof VersionNotFoundError) { - const versionList = [...error.availableVersions].reverse().join(", "); - log.warn( - "404", - `version ${error.requestedVersion} is not a valid choice of: ${versionList}` + notifyPackumentNotFoundInAnyRegistry(log, packageName); + else if (error instanceof NoVersionsError) notifyNoVersions(log, packageName); + else if (error instanceof VersionNotFoundError) + notifyOfMissingVersion( + log, + packageName, + error.requestedVersion, + error.availableVersions ); - } -} - -/** - * Logs a {@link EnvParseError} to a logger. - */ -export function logEnvParseError(log: Logger, error: EnvParseError) { - // TODO: Formulate more specific error messages. - const reason = - error instanceof NoWslError - ? "you attempted to use wsl even though you are not running openupm inside wsl" - : error instanceof GenericIOError - ? `a file-system interaction failed` - : error instanceof ChildProcessError - ? "a required child process failed" - : error instanceof RequiredEnvMissingError - ? `none of the following environment variables were set: ${error.keyNames.join( - ", " - )}` - : `a string was malformed. Expected to be ${error.formatName}`; - const errorMessage = `environment information could not be parsed because ${reason}.`; - log.error("", errorMessage); - - // TODO: Suggest actions user might take in order to fix the problem. -} - -/** - * Logs a {@link DetermineEditorVersionError} to a logger. - */ -export function logDetermineEditorError( - log: Logger, - error: DetermineEditorVersionError -) { - const reason = - error instanceof FileMissingError - ? `the projects version file (ProjectVersion.txt) could not be found at "${error.path}"` - : error instanceof GenericIOError - ? `a file-system interaction failed` - : error instanceof FileParseError - ? `the project version file (ProjectVersion.txt) has an invalid structure` - : `the project versions file (ProjectVersion.txt) did not contain valid yaml`; - - const errorMessage = `editor version could be determined because ${reason}.`; - log.error("", errorMessage); - - // TODO: Suggest actions user might take in order to fix the problem. + else if (error instanceof GenericNetworkError) + notifyRegistryCallFailedBecauseHttp(log); + else if (error instanceof RegistryAuthenticationError) + notifyRegistryCallFailedBecauseUnauthorized(log); } diff --git a/test/cli/cmd-add.test.ts b/test/cli/cmd-add.test.ts index 3abd9cc3..e79bdb0a 100644 --- a/test/cli/cmd-add.test.ts +++ b/test/cli/cmd-add.test.ts @@ -144,7 +144,7 @@ describe("cmd-add", () => { expect(log.error).toHaveBeenCalledWith( expect.any(String), - expect.stringContaining("environment information could not be parsed") + expect.stringContaining("file-system error") ); }); @@ -169,7 +169,7 @@ describe("cmd-add", () => { expect(log.error).toHaveBeenCalledWith( expect.any(String), - expect.stringContaining("editor version could be determined") + expect.stringContaining("file-system error") ); }); @@ -190,7 +190,7 @@ describe("cmd-add", () => { expect(log.error).toHaveBeenCalledWith( expect.any(String), - expect.stringContaining("could not load project manifest") + expect.stringContaining("Could not locate") ); }); @@ -210,7 +210,7 @@ describe("cmd-add", () => { await addCmd(somePackage, { _global: {} }); expect(log.error).toHaveBeenCalledWith( - "404", + expect.any(String), expect.stringContaining("not found") ); }); @@ -235,9 +235,9 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.warn).toHaveBeenCalledWith( - "404", - expect.stringContaining("is not a valid choice") + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("has no published version") ); }); @@ -400,9 +400,9 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.warn).toHaveBeenCalledWith( - "404", - expect.stringContaining("is not a valid choice") + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("has no published version") ); }); @@ -680,7 +680,10 @@ describe("cmd-add", () => { _global: {}, }); - expect(log.notice).toHaveBeenCalledWith("", expect.stringContaining("open Unity")); + expect(log.notice).toHaveBeenCalledWith( + "", + expect.stringContaining("open Unity") + ); }); it("should fail if manifest could not be saved", async () => { @@ -699,6 +702,9 @@ describe("cmd-add", () => { await addCmd(somePackage, { _global: {} }); - expect(log.error).toHaveBeenCalledWith("manifest", expect.stringContaining("")); + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("file-system error") + ); }); }); diff --git a/test/cli/cmd-remove.test.ts b/test/cli/cmd-remove.test.ts index b4f03bb3..eaf8b250 100644 --- a/test/cli/cmd-remove.test.ts +++ b/test/cli/cmd-remove.test.ts @@ -82,7 +82,10 @@ describe("cmd-remove", () => { await removeCmd(somePackage, { _global: {} }); - expect(log.error).toHaveBeenCalledWith("manifest", expect.any(String)); + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("Could not locate") + ); }); it("should fail if package version was specified", async () => { @@ -193,7 +196,10 @@ describe("cmd-remove", () => { await removeCmd(somePackage, { _global: {} }); - expect(log.error).toHaveBeenCalledWith("manifest", expect.stringContaining("")); + expect(log.error).toHaveBeenCalledWith( + expect.any(String), + expect.stringContaining("file-system error") + ); }); it("should suggest to open Unity after save", async () => { @@ -201,6 +207,9 @@ describe("cmd-remove", () => { await removeCmd(somePackage, { _global: {} }); - expect(log.notice).toHaveBeenCalledWith("", expect.stringContaining("open Unity")); + expect(log.notice).toHaveBeenCalledWith( + "", + expect.stringContaining("open Unity") + ); }); }); diff --git a/test/services/npm-login.test.ts b/test/services/npm-login.test.ts index 1c308b80..f529f446 100644 --- a/test/services/npm-login.test.ts +++ b/test/services/npm-login.test.ts @@ -1,13 +1,11 @@ import "assert"; -import { - makeNpmLoginService, -} from "../../src/services/npm-login"; +import { makeNpmLoginService } from "../../src/services/npm-login"; import RegClient from "another-npm-registry-client"; import { HttpErrorBase } from "npm-registry-fetch/lib/errors"; import { exampleRegistryUrl } from "../domain/data-registry"; import { mockRegClientAddUserResult } from "./registry-client.mock"; -import {RegistryAuthenticationError} from "../../src/io/common-errors"; -import {noopLogger} from "../../src/logging"; +import { RegistryAuthenticationError } from "../../src/io/common-errors"; +import { noopLogger } from "../../src/logging"; function makeDependencies() { const registryClient: jest.Mocked = {