fix: incorrect log (#287)

The error log for when a package version could not be found is currently incorrect. When adding package `[email protected]` and the primary registry has the package, just not that version the app will still fail with "not found". This is because `com.some.package` does not exist at all in the upstream registry and that error overrides the first "version not found". Fixed by checking which error is more fixable and printing that. A missing version is more fixable than the package not existing at all.
openupm · Apr 24, 2024 · 320fb36 · 320fb36
1 parent 139194b
commit 320fb36
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 19 deletions.
diff --git a/src/cli/cmd-add.ts b/src/cli/cmd-add.ts
@@ -30,6 +30,7 @@ import {
 import { CmdOptions } from "./options";
 import {
   PackumentResolveError,
+  pickMostFixable,
   VersionNotFoundError,
 } from "../packument-resolving";
 import { SemanticVersion } from "../domain/semantic-version";
@@ -133,12 +134,17 @@ export function makeAddCmd(
           env.registry
         ).promise;
         if (resolveResult.isErr() && env.upstream) {
-          resolveResult = await resolveRemovePackument(
+          const upstreamResult = await resolveRemovePackument(
             name,
             requestedVersion,
             env.upstreamRegistry
           ).promise;
-          if (resolveResult.isOk()) isUpstreamPackage = true;
+          if (upstreamResult.isOk()) {
+            resolveResult = upstreamResult;
+            isUpstreamPackage = true;
+          } else {
+            resolveResult = pickMostFixable(resolveResult, upstreamResult);
+          }
         }
 
         if (resolveResult.isErr()) {

diff --git a/test/cli/cmd-add.test.ts b/test/cli/cmd-add.test.ts
@@ -30,6 +30,7 @@ import {
   LoadProjectManifest,
   WriteProjectManifest,
 } from "../../src/io/project-manifest-io";
+import { makePackageReference } from "../../src/domain/package-reference";
 
 const somePackage = makeDomainName("com.some.package");
 const otherPackage = makeDomainName("com.other.package");
@@ -168,21 +169,8 @@ describe("cmd-add", () => {
     expect(errorSpy).toHaveLogLike("404", expect.stringContaining("not found"));
   });
 
-  /*
-  ---
-  TODO: Fix these tests
-  For these tests to work, logic in add-cmd needs to be changed.
-  The tests expects that in the scenario
-    - Primary registry: Has package, but not correct version
-    - Upstream registry: Does not have the package
-  it should fail because the version was not found.
-  
-  Currently it will still fail with "package not found" because the error
-  of the package missing in the upstream overrides the first error.
-  ---
-      
   it("should fail if package version could not be resolved", async () => {
-    const {addCmd} = makeDependencies();
+    const { addCmd } = makeDependencies();
 
     const result = await addCmd(makePackageReference(somePackage, "2.0.0"), {
       _global: {},
@@ -195,15 +183,17 @@ describe("cmd-add", () => {
 
   it("should notify if package version could not be resolved", async () => {
     const warnSpy = spyOnLog("warn");
-    const {addCmd} = makeDependencies();
+    const { addCmd } = makeDependencies();
 
     await addCmd(makePackageReference(somePackage, "2.0.0"), {
       _global: {},
     });
 
-    expect(warnSpy).toHaveLogLike("404", expect.stringContaining("is not a valid choice"));
+    expect(warnSpy).toHaveLogLike(
+      "404",
+      expect.stringContaining("is not a valid choice")
+    );
   });
-  */
 
   it("should notify if editor-version is unknown", async () => {
     const warnSpy = spyOnLog("warn");