diff --git a/go.mod b/go.mod index 9462145f9..fb0c8447f 100644 --- a/go.mod +++ b/go.mod @@ -19,10 +19,10 @@ require ( github.com/buger/jsonparser v1.3.0 github.com/gocarina/gocsv v0.0.0-20260607070740-0735908c6461 github.com/jfrog/archiver/v3 v3.6.5 - github.com/jfrog/build-info-go v1.13.1-0.20260910072358-fc0223006a3b + github.com/jfrog/build-info-go v1.13.1-0.20260916041531-17e0b8fec4e5 github.com/jfrog/gofrog v1.7.7 github.com/jfrog/jfrog-cli-application v1.0.2-0.20260820134442-c8629258ff3a - github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260910080031-983313bcbc6e + github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260916050638-7c7bafc306e6 github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260909093400-32a7208a18bd github.com/jfrog/jfrog-cli-evidence v0.11.1-0.20260824063609-79b735ec565e github.com/jfrog/jfrog-cli-platform-services v1.10.1-0.20260618062042-6053ab368cab diff --git a/go.sum b/go.sum index a8b0dd7a2..78248b4f5 100644 --- a/go.sum +++ b/go.sum @@ -390,8 +390,8 @@ github.com/jellydator/ttlcache/v3 v3.4.0 h1:YS4P125qQS0tNhtL6aeYkheEaB/m8HCqdMMP github.com/jellydator/ttlcache/v3 v3.4.0/go.mod h1:Hw9EgjymziQD3yGsQdf1FqFdpp7YjFMd4Srg5EJlgD4= github.com/jfrog/archiver/v3 v3.6.5 h1:AiNXJoe8jYDOtyykfVuwh26aM4rk/ei+YzBpfBukdzU= github.com/jfrog/archiver/v3 v3.6.5/go.mod h1:5V9l+Fte30Y4qe9dUOAd3yNTf8lmtVNuhKNrvI8PMhg= -github.com/jfrog/build-info-go v1.13.1-0.20260910072358-fc0223006a3b h1:DEHE5lr01Yq7zRkwyzdrBGlhvVWPi6W8o46jv0AT8/Y= -github.com/jfrog/build-info-go v1.13.1-0.20260910072358-fc0223006a3b/go.mod h1:CYRUCvLKfyARjoJXLWAxce1qNUxTEtbRKAARkV42vpE= +github.com/jfrog/build-info-go v1.13.1-0.20260916041531-17e0b8fec4e5 h1:9VObKIxffivhEWRDot/L8f2RErHWeVV+ZTHXFxnxAbk= +github.com/jfrog/build-info-go v1.13.1-0.20260916041531-17e0b8fec4e5/go.mod h1:CYRUCvLKfyARjoJXLWAxce1qNUxTEtbRKAARkV42vpE= github.com/jfrog/froggit-go v1.23.1 h1:4wmaHeuptxVINbovMaeITzVhi3+VQoc/FFIjF4axzu0= github.com/jfrog/froggit-go v1.23.1/go.mod h1:wRDryqyp3oe+eHgME2mpnEQmO8XBECIPagFwj0nHmdI= github.com/jfrog/go-mockhttp v0.3.1 h1:/wac8v4GMZx62viZmv4wazB5GNKs+GxawuS1u3maJH8= @@ -402,8 +402,8 @@ github.com/jfrog/jfrog-apps-config v1.0.1 h1:mtv6k7g8A8BVhlHGlSveapqf4mJfonwvXYL github.com/jfrog/jfrog-apps-config v1.0.1/go.mod h1:8AIIr1oY9JuH5dylz2S6f8Ym2MaadPLR6noCBO4C22w= github.com/jfrog/jfrog-cli-application v1.0.2-0.20260820134442-c8629258ff3a h1:7GhcPfi+k9oOAJdCsKWjymnqH0e7DQZ1soVhbneYnGY= github.com/jfrog/jfrog-cli-application v1.0.2-0.20260820134442-c8629258ff3a/go.mod h1:p8yLtbmCxxQucIbLZKnWu0F+EDtj6NLXbRQCEK/nb6o= -github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260910080031-983313bcbc6e h1:/zfbJFo/FsvZptWuOxauzdtArdZmgbRfnoSUtOhf3Z8= -github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260910080031-983313bcbc6e/go.mod h1:sBP/2ovBQ5R2WyJ0shm3JkNNh4IpnVg7pS4sSoJlC1M= +github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260916050638-7c7bafc306e6 h1:cQX2dOOPgi1qF27QuVyQvdzuz/HcrZx5JiwaR510IRA= +github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260916050638-7c7bafc306e6/go.mod h1:+uaehBILsa1CQiIEM+FAnVj4X7EIvmHB4n901AfivAY= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260909093400-32a7208a18bd h1:tfC6CtOpqWoU/1V4ymBL0GnhM2kbq5JTGSaAk9eOybg= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260909093400-32a7208a18bd/go.mod h1:SwV+DNLBnWLxBeNeZpJk+xxAbqJ8ywq1va56up+AGu4= github.com/jfrog/jfrog-cli-evidence v0.11.1-0.20260824063609-79b735ec565e h1:+QYbewvK+PZKbfPpxYmy0bewhqMFtJPk/tUbCICjf8U= diff --git a/npm_test.go b/npm_test.go index 38c0ddf37..2614fb64e 100644 --- a/npm_test.go +++ b/npm_test.go @@ -482,6 +482,56 @@ func initNpmProjectTest(t *testing.T) (npmProjectPath string) { return } +// initNpmFailOnUncollectedDepsProjectTest sets up the npmfailonuncollecteddeps fixture: a regular dependency +// ("xml") plus an optionalDependency ("json"), so the cache-corruption technique used by +// TestNpmFailOnUncollectedDepsErrorFormat can reproduce a genuine uncollected-dependency scenario for +// "optional" as well as "regular" (npmproject, used elsewhere in this file, has no optional deps at +// all). "peer" and "bundle" are deliberately not reproduced here - see the fixture's package.json. +func initNpmFailOnUncollectedDepsProjectTest(t *testing.T) (npmProjectPath string) { + npmProjectPath = filepath.Dir(createNpmProject(t, "npmfailonuncollecteddeps")) + err := createConfigFileForTest([]string{npmProjectPath}, tests.NpmRemoteRepo, tests.NpmRepo, t, project.Npm, false) + assert.NoError(t, err) + prepareArtifactoryForNpmBuild(t, npmProjectPath) + return +} + +// initNpmFailOnUncollectedBundleProjectTest copies the packed-bundle fixture. The child +// (bundled-pkg) declares xml as bundleDependencies; the app depends on a tarball produced +// at runtime by packBundledPkgWithXml — testdata does not check in a .tgz. +func initNpmFailOnUncollectedBundleProjectTest(t *testing.T) (appPath, bundledPkgPath string) { + src := filepath.Join(filepath.FromSlash(tests.GetTestResourcesPath()), "npm", "npmfailonuncollectedbundle") + appPath = filepath.Join(tests.Out, "npmfailonuncollectedbundle") + require.NoError(t, biutils.CopyDir(src, appPath, true, nil)) + var err error + appPath, err = filepath.Abs(appPath) + require.NoError(t, err) + bundledPkgPath = filepath.Join(appPath, "bundled-pkg") + err = createConfigFileForTest([]string{appPath, bundledPkgPath}, tests.NpmRemoteRepo, tests.NpmRepo, t, project.Npm, false) + require.NoError(t, err) + return +} + +// packBundledPkgWithXml installs xml into bundled-pkg from Artifactory, packs it so xml is +// nested with _inBundle, and copies the tarball next to the app package.json. +func packBundledPkgWithXml(t *testing.T, bundledPkgPath, appPath string) { + wd, err := os.Getwd() + require.NoError(t, err) + chdir := clientTestUtils.ChangeDirWithCallback(t, wd, bundledPkgPath) + defer chdir() + + require.NoError(t, runJfrogCliWithoutAssertion("npm", "install"), "install xml into the package that will be packed") + + cmd := exec.Command("npm", "pack") + cmd.Dir = bundledPkgPath + out, err := cmd.CombinedOutput() + require.NoError(t, err, "npm pack failed: %s", out) + + matches, err := filepath.Glob(filepath.Join(bundledPkgPath, "bundled-pkg-*.tgz")) + require.NoError(t, err) + require.Len(t, matches, 1, "expected one packed tarball, got %v", matches) + require.NoError(t, biutils.CopyFile(appPath, matches[0])) +} + func initNpmWorkspacesProjectTest(t *testing.T) (npmProjectPath string) { npmProjectPath = filepath.Dir(createNpmProject(t, "npmworkspaces")) err := createConfigFileForTest([]string{npmProjectPath}, tests.NpmRemoteRepo, tests.NpmRepo, t, project.Npm, false) @@ -1686,3 +1736,737 @@ func TestNpmPublishWithLocalGitVcsProps(t *testing.T) { tests.VcsFixtureMainURL, tests.VcsFixtureMainRevision, tests.VcsFixtureMainBranch) assert.Greater(t, count, 0) } + +// TestNpmFailOnUncollectedDeps - COMPREHENSIVE SUITE +// Tests all permutations and combinations of --fail-on-uncollected-deps flag +// +// SUCCESS PATHS (what we test end-to-end with real apmtest server): +// - Backward compatibility (no flag) +// - All individual flag values: all, peer, optional, regular, bundle +// - 6 permutations/combinations of 2+ flags (excludes 'all' combined with another value - that's +// rejected as invalid, see the NEGATIVE cases below) +// - 3 semantic edge cases verifying exclusion logic +// +// Total: 13 subtests covering all realistic success scenarios +// +// IMPORTANT: every case above expects success. This project (npmproject, shared with most other npm +// tests in this file) declares no peer, bundle, or optional dependencies at all, so setting +// --fail-on-uncollected-deps=peer/bundle/all etc. here can never actually catch anything - these +// subtests only prove the flag doesn't false-positive on an otherwise-healthy install, not that it +// correctly detects and fails on a real missing dependency of those types. +// +// Real detection coverage: +// - "regular" and "optional": TestNpmFailOnUncollectedDepsErrorFormat, using cache corruption +// (populate cache -> wipe tarballs -> reinstall) against testdata/npm/npmfailonuncollecteddeps. +// - "bundle": TestNpmFailOnUncollectedDepsBundle, using a packed local tarball that ships xml +// as bundleDependencies so npm ls reports _inBundle without integrity. +// - "peer": not reproduced end-to-end here. PeerMissing is the npm v6 ls field; CLI npm CI is +// Node 16 / npm 8. See build-info-go TestHandleMissingDeps and TestConflictsDependenciesList. +func TestNpmFailOnUncollectedDeps(t *testing.T) { + initNpmTest(t) + defer cleanNpmTest(t) + + wd, err := os.Getwd() + assert.NoError(t, err, "Failed to get current dir") + defer clientTestUtils.ChangeDirAndAssert(t, wd) + + _, _, err = buildutils.GetNpmVersionAndExecPath(log.Logger) + if err != nil { + assert.NoError(t, err, "npm must be available for this test") + return + } + + testCases := []struct { + name string + flagValue string + buildName string + buildNumber string + expectedSuccess bool + description string + category string // "backward_compat", "individual", "combo", "semantic", "negative" + expectedErrorHint string // for category "negative": substring the validation error must contain + }{ + // ===== 1. BACKWARD COMPATIBILITY ===== + { + name: "backward_compat_no_flag", + flagValue: "", + buildName: "npm-no-flag", + buildNumber: "1", + expectedSuccess: true, + description: "Without flag: warns but doesn't fail (backward compat preserved)", + category: "backward_compat", + }, + + // ===== 2. INDIVIDUAL FLAG VALUES (5 tests) ===== + { + name: "flag_all", + flagValue: "all", + buildName: "npm-flag-all", + buildNumber: "1", + expectedSuccess: true, + description: "Flag: all (monitors all 4 dep types)", + category: "individual", + }, + { + name: "flag_peer", + flagValue: "peer", + buildName: "npm-flag-peer", + buildNumber: "1", + expectedSuccess: true, + description: "Flag: peer (peerDependencies only)", + category: "individual", + }, + { + name: "flag_optional", + flagValue: "optional", + buildName: "npm-flag-optional", + buildNumber: "1", + expectedSuccess: true, + description: "Flag: optional (optionalDependencies only)", + category: "individual", + }, + { + name: "flag_regular", + flagValue: "regular", + buildName: "npm-flag-regular", + buildNumber: "1", + expectedSuccess: true, + description: "Flag: regular (regular/dev/bundle, NOT optional)", + category: "individual", + }, + { + name: "flag_bundle", + flagValue: "bundle", + buildName: "npm-flag-bundle", + buildNumber: "1", + expectedSuccess: true, + description: "Flag: bundle (bundleDependencies only)", + category: "individual", + }, + + // ===== 3. PERMUTATIONS & COMBINATIONS (8 tests) ===== + // 2-flag combinations + { + name: "combo_peer_optional", + flagValue: "peer,optional", + buildName: "npm-combo-peer-opt", + buildNumber: "1", + expectedSuccess: true, + description: "Combo: peer + optional (2-way combination)", + category: "combo", + }, + { + name: "combo_peer_bundle", + flagValue: "peer,bundle", + buildName: "npm-combo-peer-bundle", + buildNumber: "1", + expectedSuccess: true, + description: "Combo: peer + bundle (2-way combination)", + category: "combo", + }, + { + name: "combo_optional_bundle", + flagValue: "optional,bundle", + buildName: "npm-combo-opt-bundle", + buildNumber: "1", + expectedSuccess: true, + description: "Combo: optional + bundle (2-way combination)", + category: "combo", + }, + { + name: "combo_regular_optional", + flagValue: "regular,optional", + buildName: "npm-combo-reg-opt", + buildNumber: "1", + expectedSuccess: true, + description: "Combo: regular + optional (2-way combination)", + category: "combo", + }, + // 3-flag combinations + { + name: "combo_peer_optional_bundle", + flagValue: "peer,optional,bundle", + buildName: "npm-combo-trio", + buildNumber: "1", + expectedSuccess: true, + description: "Combo: peer + optional + bundle (3-way combination)", + category: "combo", + }, + { + name: "combo_regular_peer_bundle", + flagValue: "regular,peer,bundle", + buildName: "npm-combo-reg-peer-bundle", + buildNumber: "1", + expectedSuccess: true, + description: "Combo: regular + peer + bundle (3-way, no overlap)", + category: "combo", + }, + + // ===== 4. SEMANTIC CORRECTNESS - EDGE CASES (3 tests) ===== + // These verify that flags correctly EXCLUDE certain dependency types + { + name: "semantic_regular_excludes_optional", + flagValue: "regular", + buildName: "npm-sem-reg-excl-opt", + buildNumber: "1", + expectedSuccess: true, + description: "Semantic: 'regular' flag correctly EXCLUDES optional deps from monitoring", + category: "semantic", + }, + { + name: "semantic_optional_excludes_regular", + flagValue: "optional", + buildName: "npm-sem-opt-excl-reg", + buildNumber: "1", + expectedSuccess: true, + description: "Semantic: 'optional' flag ONLY monitors optional deps (excludes regular)", + category: "semantic", + }, + { + name: "semantic_peer_excludes_optional", + flagValue: "peer", + buildName: "npm-sem-peer-excl-opt", + buildNumber: "1", + expectedSuccess: true, + description: "Semantic: 'peer' flag correctly EXCLUDES optional deps from monitoring", + category: "semantic", + }, + + // ===== NEGATIVE SCENARIOS (Invalid Inputs) ===== + { + name: "invalid_flag_unknown_value", + flagValue: "invalid", + buildName: "npm-invalid-flag", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: unknown flag value 'invalid'", + category: "negative", + expectedErrorHint: "invalid", + }, + { + name: "invalid_flag_case_sensitive_ALL", + flagValue: "ALL", + buildName: "npm-case-ALL", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: flag is case-sensitive ('ALL' not valid, must be 'all')", + category: "negative", + expectedErrorHint: "invalid", + }, + { + name: "invalid_flag_malformed_trailing_comma", + flagValue: "peer,", + buildName: "npm-malformed-trailing", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: malformed flag with trailing comma 'peer,'", + category: "negative", + expectedErrorHint: "invalid", + }, + { + name: "invalid_flag_malformed_leading_comma", + flagValue: ",peer", + buildName: "npm-malformed-leading", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: malformed flag with leading comma ',peer'", + category: "negative", + expectedErrorHint: "invalid", + }, + { + name: "invalid_flag_double_comma", + flagValue: "peer,,bundle", + buildName: "npm-double-comma", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: malformed flag with double comma 'peer,,bundle'", + category: "negative", + expectedErrorHint: "invalid", + }, + { + name: "invalid_flag_special_chars", + flagValue: "peer@bundle", + buildName: "npm-special-chars", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: flag with special characters 'peer@bundle'", + category: "negative", + expectedErrorHint: "invalid", + }, + { + name: "invalid_flag_all_combined_with_peer", + flagValue: "all,peer", + buildName: "npm-combo-all-peer", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: 'all' combined with another value 'all,peer'", + category: "negative", + expectedErrorHint: "cannot be combined", + }, + { + name: "invalid_flag_all_combined_with_optional_bundle", + flagValue: "all,optional,bundle", + buildName: "npm-combo-all-opt-bundle", + buildNumber: "1", + expectedSuccess: false, + description: "Should reject: 'all' combined with other values 'all,optional,bundle'", + category: "negative", + expectedErrorHint: "cannot be combined", + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + inttestutils.DeleteBuild(serverDetails.ArtifactoryUrl, tt.buildName, artHttpDetails) + defer inttestutils.DeleteBuild(serverDetails.ArtifactoryUrl, tt.buildName, artHttpDetails) + + projectPath := initNpmProjectTest(t) + chdirCallBack := clientTestUtils.ChangeDirWithCallback(t, wd, projectPath) + defer chdirCallBack() + + switch tt.category { + case "error_format": + // ===== ERROR FORMAT TESTS: Actually recreate missing dependency scenarios ===== + // Pattern: useIsolatedCache → install (populate) → wipeCache (corrupt) → install (detect missing) + + cacheDir, restoreCache := useIsolatedNpmCache(t) + defer restoreCache() + + // STEP 1: Initial install to populate the isolated cache + installArgs := []string{"npm", "install", "--cache=" + cacheDir} + initialErr := runJfrogCliWithoutAssertion(installArgs...) + assert.NoError(t, initialErr, "Initial cache population should succeed for: %s", tt.description) + + // STEP 2: Corrupt the cache by removing tarballs to simulate missing dependencies + wipeNpmCacacheTarballs(t, cacheDir) + + // STEP 3: Second install with flag should fail because tarballs are missing + args := []string{"npm", "install", "--cache=" + cacheDir, + "--build-name=" + tt.buildName, "--build-number=" + tt.buildNumber, + "--fail-on-uncollected-deps=" + tt.flagValue} + + err := runJfrogCliWithoutAssertion(args...) + + // STEP 4: Verify error occurs and message is properly formatted + assert.Error(t, err, tt.description) + if err != nil { + errMsg := err.Error() + // Verify error message contains appropriate hints based on missing deps type + if strings.Contains(tt.flagValue, "regular") || tt.flagValue == "all" { + // Should contain npm cache hint for regular deps + assert.Contains(t, errMsg, "npm cache", + "Error should mention npm cache for regular deps: %s", tt.description) + } + if tt.flagValue != "regular" && tt.flagValue != "" { + // Should contain npm ls hint for peer/bundle/optional + assert.Contains(t, errMsg, "npm ls", + "Error should mention npm ls for peer/bundle/optional: %s", tt.description) + } + } + t.Logf("[PASS-%s] %s (error recreated with isolated cache corruption)", strings.ToUpper(tt.category), tt.description) + + case "negative": + // ===== NEGATIVE TESTS: Invalid flag values should be rejected ===== + args := []string{"npm", "install", "--build-name=" + tt.buildName, "--build-number=" + tt.buildNumber, + "--fail-on-uncollected-deps=" + tt.flagValue} + + err := runJfrogCliWithoutAssertion(args...) + // Negative test case: should fail with validation error + assert.Error(t, err, tt.description) + if err != nil { + assert.Contains(t, err.Error(), tt.expectedErrorHint, "Error should mention '%s': %s", tt.expectedErrorHint, tt.description) + } + t.Logf("[PASS-%s] %s (correctly rejected with validation error)", strings.ToUpper(tt.category), tt.description) + + default: + // ===== SUCCESS PATH TESTS: Normal flow with valid flags ===== + if !tt.expectedSuccess { + return + } + args := []string{"npm", "install", "--build-name=" + tt.buildName, "--build-number=" + tt.buildNumber} + if tt.flagValue != "" { + args = append(args, "--fail-on-uncollected-deps="+tt.flagValue) + } + + err := runJfrogCliWithoutAssertion(args...) + assert.NoError(t, err, tt.description) + + // Publish build info + assert.NoError(t, artifactoryCli.Exec("bp", tt.buildName, tt.buildNumber), + "Failed to publish build for: %s", tt.buildName) + + // Verify build info exists and contains modules + publishedBuildInfo, found, err := tests.GetBuildInfo(serverDetails, tt.buildName, tt.buildNumber) + assert.NoError(t, err) + assert.True(t, found, "Build info should exist: %s", tt.description) + if assert.NotNil(t, publishedBuildInfo) && assert.NotNil(t, publishedBuildInfo.BuildInfo) { + assert.Greater(t, len(publishedBuildInfo.BuildInfo.Modules), 0, + "Modules should be present: %s", tt.description) + } + t.Logf("[PASS-%s] %s", strings.ToUpper(tt.category), tt.description) + } + + clientTestUtils.ChangeDirAndAssert(t, wd) + }) + } +} + +// useIsolatedNpmCache points npm at a dedicated cache directory via npm_config_cache. +// Callers must also pass --cache= to every npm invocation: the env var alone loses to an +// NPM_CONFIG_CACHE already exported by the environment, which makes 'npm config get cache' +// (how the build-info collector locates the cache) report a directory the test never wiped. +func useIsolatedNpmCache(t *testing.T) (cacheDir string, restore func()) { + cacheDir = t.TempDir() + return cacheDir, clientTestUtils.SetEnvWithCallbackAndAssert(t, "npm_config_cache", cacheDir) +} + +// npmCachedTarballs lists the content-v2 tarballs in the cache, relative to cacheDir. +func npmCachedTarballs(cacheDir string) []string { + contentPath := filepath.Join(cacheDir, "_cacache", "content-v2") + entries, err := os.ReadDir(contentPath) + if err != nil { + return nil + } + tarballs := []string{} + for _, entry := range entries { + if entry.IsDir() { + subentries, err := os.ReadDir(filepath.Join(contentPath, entry.Name())) + if err != nil { + continue + } + for _, subentry := range subentries { + tarballs = append(tarballs, filepath.Join(contentPath, entry.Name(), subentry.Name())) + } + } + } + return tarballs +} + +// wipeNpmCacacheTarballs removes cached tarballs and index-v5 so xml/json cannot be checksummed. +// GetNpmConfigCache requires _cacache to exist; node_modules is left in place so the next +// npm install stays up to date and does not refill the cache from the registry. +func wipeNpmCacacheTarballs(t *testing.T, cacheDir string) { + cacachePath := filepath.Join(cacheDir, "_cacache") + tarballs := npmCachedTarballs(cacheDir) + require.NotEmpty(t, tarballs, "cache should hold tarballs before wiping, otherwise the test proves nothing") + require.NoError(t, os.RemoveAll(filepath.Join(cacachePath, "content-v2"))) + require.NoError(t, os.RemoveAll(filepath.Join(cacachePath, "index-v5"))) + require.NoError(t, os.MkdirAll(cacachePath, 0755)) +} + +// TestNpmFailOnUncollectedDepsNegative tests invalid flag values and error handling. +// These tests verify that the flag validation rejects malformed input with clear error messages. +func TestNpmFailOnUncollectedDepsNegative(t *testing.T) { + initNpmTest(t) + defer cleanNpmTest(t) + + wd, err := os.Getwd() + require.NoError(t, err) + defer clientTestUtils.ChangeDirAndAssert(t, wd) + + _, _, err = buildutils.GetNpmVersionAndExecPath(log.Logger) + if err != nil { + assert.NoError(t, err, "npm must be available for this test") + return + } + + testCases := []struct { + name string + flagValue string + buildName string + buildNumber string + description string + }{ + { + name: "invalid_unknown_value", + flagValue: "invalid", + buildName: "npm-invalid-value", + buildNumber: "1", + description: "Should reject unknown flag value 'invalid'", + }, + { + name: "invalid_case_sensitive", + flagValue: "ALL", + buildName: "npm-case-all", + buildNumber: "1", + description: "Should reject case-insensitive 'ALL' (must be 'all')", + }, + { + name: "invalid_trailing_comma", + flagValue: "peer,", + buildName: "npm-trailing-comma", + buildNumber: "1", + description: "Should reject trailing comma 'peer,'", + }, + { + name: "invalid_leading_comma", + flagValue: ",peer", + buildName: "npm-leading-comma", + buildNumber: "1", + description: "Should reject leading comma ',peer'", + }, + { + name: "invalid_double_comma", + flagValue: "peer,,bundle", + buildName: "npm-double-comma", + buildNumber: "1", + description: "Should reject double comma 'peer,,bundle'", + }, + { + name: "invalid_special_chars", + flagValue: "peer@bundle", + buildName: "npm-special-chars", + buildNumber: "1", + description: "Should reject special characters 'peer@bundle'", + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + projectPath := initNpmProjectTest(t) + chdirCallBack := clientTestUtils.ChangeDirWithCallback(t, wd, projectPath) + defer chdirCallBack() + + args := []string{"npm", "install", + "--build-name=" + tt.buildName, + "--build-number=" + tt.buildNumber, + "--fail-on-uncollected-deps=" + tt.flagValue} + + err := runJfrogCliWithoutAssertion(args...) + // Should fail with validation error + assert.Error(t, err, tt.description) + if err != nil { + assert.Contains(t, err.Error(), "invalid", "Error should mention 'invalid' for: %s", tt.description) + } + t.Logf("[PASS-NEGATIVE] %s", tt.description) + + clientTestUtils.ChangeDirAndAssert(t, wd) + }) + } +} + +// TestNpmFailOnUncollectedDepsRequiresBuildInfo verifies that --fail-on-uncollected-deps, given a +// value, requires --build-name/--build-number - mirroring --module's existing requirement on them. +// Without this, the flag would be silently ignored (build-info collection, where it operates, never +// runs). The actual value/build-tracking logic is unit-tested directly in jfrog-cli-artifactory +// (TestValidateFailOnUncollectedDepsBuild); this proves Init() actually wires that check in and the +// error reaches the user through the real command path. +func TestNpmFailOnUncollectedDepsRequiresBuildInfo(t *testing.T) { + initNpmTest(t) + defer cleanNpmTest(t) + wd, err := os.Getwd() + assert.NoError(t, err) + defer clientTestUtils.ChangeDirAndAssert(t, wd) + + projectPath := initNpmProjectTest(t) + chdirCallBack := clientTestUtils.ChangeDirWithCallback(t, wd, projectPath) + defer chdirCallBack() + + err = runJfrogCliWithoutAssertion("npm", "install", "--fail-on-uncollected-deps=all") + assert.Error(t, err) + if err != nil { + assert.Contains(t, err.Error(), "mandatory") + } +} + +// TestNpmFailOnUncollectedDepsErrorFormat tests error message formatting when dependencies are missing. +// Uses isolated cache corruption to actually recreate missing dependency scenarios. +func TestNpmFailOnUncollectedDepsErrorFormat(t *testing.T) { + initNpmTest(t) + defer cleanNpmTest(t) + + wd, err := os.Getwd() + require.NoError(t, err) + defer clientTestUtils.ChangeDirAndAssert(t, wd) + + _, _, err = buildutils.GetNpmVersionAndExecPath(log.Logger) + if err != nil { + assert.NoError(t, err, "npm must be available for this test") + return + } + + testCases := []struct { + name string + // useOptionalDepsFixture switches to a fixture with a real optionalDependency + // (npmproject, used by default, declares no optional deps at all, so there'd be + // nothing for cache corruption to make "missing"). + useOptionalDepsFixture bool + flagValue string + buildName string + buildNumber string + expectHints []string // Expected hints in error message + description string + }{ + { + name: "error_regular_deps", + flagValue: "regular", + buildName: "npm-err-regular", + buildNumber: "1", + expectHints: []string{"npm cache"}, + description: "Error should mention npm cache for regular deps", + }, + { + name: "error_optional_deps", + useOptionalDepsFixture: true, + flagValue: "optional", + buildName: "npm-err-optional", + buildNumber: "1", + expectHints: []string{"npm cache"}, + description: "Error should mention npm cache for optional deps", + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + var projectPath string + if tt.useOptionalDepsFixture { + projectPath = initNpmFailOnUncollectedDepsProjectTest(t) + } else { + projectPath = initNpmProjectTest(t) + } + chdirCallBack := clientTestUtils.ChangeDirWithCallback(t, wd, projectPath) + defer chdirCallBack() + + // ===== RECREATE ERROR SCENARIO ===== + // STEP 1: Create isolated cache + cacheDir, restoreCache := useIsolatedNpmCache(t) + defer restoreCache() + + // STEP 2: Initial install to populate cache + installArgs := []string{"npm", "install", "--cache=" + cacheDir} + initialErr := runJfrogCliWithoutAssertion(installArgs...) + assert.NoError(t, initialErr, "Cache population should succeed") + + // STEP 3: Corrupt cache to simulate missing dependencies + wipeNpmCacacheTarballs(t, cacheDir) + + // STEP 4: Run with flag - should fail with missing deps error + args := []string{"npm", "install", "--cache=" + cacheDir, + "--build-name=" + tt.buildName, + "--build-number=" + tt.buildNumber, + "--fail-on-uncollected-deps=" + tt.flagValue} + + err := runJfrogCliWithoutAssertion(args...) + + // Verify error occurs and has proper hints + assert.Error(t, err, tt.description) + if err != nil { + errMsg := err.Error() + for _, hint := range tt.expectHints { + assert.Contains(t, errMsg, hint, "Error should mention '%s' for: %s", hint, tt.description) + } + } + t.Logf("[PASS-ERROR-FORMAT] %s", tt.description) + + clientTestUtils.ChangeDirAndAssert(t, wd) + }) + } +} + +// TestNpmFailOnUncollectedDepsBundle reproduces a real bundleDependencies miss: a packed +// local tarball that ships xml inside it. npm ls reports that nested xml as _inBundle with +// no integrity, which is the bundle bucket — not a cache wipe (that path is regular/optional). +func TestNpmFailOnUncollectedDepsBundle(t *testing.T) { + initNpmTest(t) + defer cleanNpmTest(t) + + wd, err := os.Getwd() + require.NoError(t, err) + defer clientTestUtils.ChangeDirAndAssert(t, wd) + + _, _, err = buildutils.GetNpmVersionAndExecPath(log.Logger) + if err != nil { + assert.NoError(t, err, "npm must be available for this test") + return + } + + buildName := "npm-err-bundle" + buildNumber := "1" + inttestutils.DeleteBuild(serverDetails.ArtifactoryUrl, buildName, artHttpDetails) + defer inttestutils.DeleteBuild(serverDetails.ArtifactoryUrl, buildName, artHttpDetails) + + appPath, bundledPkgPath := initNpmFailOnUncollectedBundleProjectTest(t) + packBundledPkgWithXml(t, bundledPkgPath, appPath) + + chdir := clientTestUtils.ChangeDirWithCallback(t, wd, appPath) + defer chdir() + + cacheDir, restoreCache := useIsolatedNpmCache(t) + defer restoreCache() + + err = runJfrogCliWithoutAssertion("npm", "install", "--cache="+cacheDir, + "--build-name="+buildName, + "--build-number="+buildNumber, + "--fail-on-uncollected-deps=bundle") + require.Error(t, err, "packed bundleDependencies with empty integrity should fail collection when the flag targets bundle") + errMsg := err.Error() + assert.Contains(t, errMsg, "Build-info collection stopped") + assert.Contains(t, errMsg, "bundleDependencies") + assert.Contains(t, errMsg, "npm ls") + assert.Contains(t, errMsg, "integrity") +} + +// TestNpmMissingDepsLegacyBehavior tests backward compatibility: without the flag, missing deps generate debug/warn logs but don't fail. +// This ensures existing workflows that don't use the flag continue to work as before. +func TestNpmMissingDepsLegacyBehavior(t *testing.T) { + initNpmTest(t) + defer cleanNpmTest(t) + + wd, err := os.Getwd() + require.NoError(t, err) + defer clientTestUtils.ChangeDirAndAssert(t, wd) + + _, _, err = buildutils.GetNpmVersionAndExecPath(log.Logger) + if err != nil { + assert.NoError(t, err, "npm must be available for this test") + return + } + + t.Run("no_flag_with_missing_deps", func(t *testing.T) { + buildName := "npm-legacy-warn" + buildNumber := "1" + + inttestutils.DeleteBuild(serverDetails.ArtifactoryUrl, buildName, artHttpDetails) + defer inttestutils.DeleteBuild(serverDetails.ArtifactoryUrl, buildName, artHttpDetails) + + projectPath := initNpmProjectTest(t) + chdirCallBack := clientTestUtils.ChangeDirWithCallback(t, wd, projectPath) + defer chdirCallBack() + + // Setup isolated cache and corrupt it + cacheDir, restoreCache := useIsolatedNpmCache(t) + defer restoreCache() + + // Initial install to populate cache + installArgs := []string{"npm", "install", "--cache=" + cacheDir} + initialErr := runJfrogCliWithoutAssertion(installArgs...) + require.NoError(t, initialErr, "Cache population should succeed") + + // Corrupt cache + wipeNpmCacacheTarballs(t, cacheDir) + + // WITHOUT --fail-on-uncollected-deps flag: should succeed (legacy behavior - warns/logs but doesn't fail) + args := []string{"npm", "install", "--cache=" + cacheDir, + "--build-name=" + buildName, + "--build-number=" + buildNumber} + + err := runJfrogCliWithoutAssertion(args...) + // Legacy behavior: should NOT fail even with missing deps + assert.NoError(t, err, "WITHOUT flag: missing deps should warn but NOT fail (legacy behavior)") + + // Verify build-info was still published (partial build info is OK without the flag) + clientTestUtils.ChangeDirAndAssert(t, wd) + publishErr := artifactoryCli.Exec("bp", buildName, buildNumber) + // May or may not succeed depending on whether build-info was collected, but the install itself should have succeeded + if publishErr == nil { + publishedBuildInfo, found, err := tests.GetBuildInfo(serverDetails, buildName, buildNumber) + assert.NoError(t, err) + if found && publishedBuildInfo != nil { + // Build info exists (may be partial without strict mode) + assert.NotNil(t, publishedBuildInfo.BuildInfo, "Build info should be populated") + } + } + + t.Logf("[PASS-LEGACY] Without flag: missing deps warn/log but don't fail (backward compat preserved)") + }) +} diff --git a/testdata/npm/npmfailonuncollectedbundle/bundled-pkg/package.json b/testdata/npm/npmfailonuncollectedbundle/bundled-pkg/package.json new file mode 100644 index 000000000..cf5f23a50 --- /dev/null +++ b/testdata/npm/npmfailonuncollectedbundle/bundled-pkg/package.json @@ -0,0 +1,11 @@ +{ + "name": "bundled-pkg", + "version": "1.0.0", + "license": "ISC", + "dependencies": { + "xml": "1.0.1" + }, + "bundleDependencies": [ + "xml" + ] +} diff --git a/testdata/npm/npmfailonuncollectedbundle/package.json b/testdata/npm/npmfailonuncollectedbundle/package.json new file mode 100644 index 000000000..0d65666b6 --- /dev/null +++ b/testdata/npm/npmfailonuncollectedbundle/package.json @@ -0,0 +1,8 @@ +{ + "name": "npm-fail-on-uncollected-bundle-test", + "version": "1.0.0", + "license": "ISC", + "dependencies": { + "bundled-pkg": "file:./bundled-pkg-1.0.0.tgz" + } +} diff --git a/testdata/npm/npmfailonuncollecteddeps/package.json b/testdata/npm/npmfailonuncollecteddeps/package.json new file mode 100644 index 000000000..0487bca18 --- /dev/null +++ b/testdata/npm/npmfailonuncollecteddeps/package.json @@ -0,0 +1,18 @@ +{ + "name": "npm-fail-on-uncollected-deps-test", + "version": "1.0.0", + "description": "Fixture for --fail-on-uncollected-deps: a regular dependency ('xml') and an optionalDependency ('json'), both reused from testdata/npm/npmproject where they're already known to resolve cleanly. See TestNpmFailOnUncollectedDepsErrorFormat in npm_test.go for why 'peer' and 'bundle' aren't reproduced here: an unmet peerDependency tends to abort 'npm install' itself (via an ERESOLVE conflict) before build-info collection ever runs, and bundleDependencies only affects 'npm pack'/'publish' of this package, not npm ls's reporting of a normally-installed one.", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "keywords": [], + "author": "", + "license": "ISC", + "dependencies": { + "xml": "1.0.1" + }, + "optionalDependencies": { + "json": "9.0.6" + } +} diff --git a/uv_test.go b/uv_test.go index 97e85635c..a43df63df 100644 --- a/uv_test.go +++ b/uv_test.go @@ -1524,7 +1524,7 @@ func TestUvPublishWithLocalGitVcsProps(t *testing.T) { require.NoError(t, runUvCmd(t, projectPath, "build")) require.NoError(t, runUvCmd(t, projectPath, "publish", "--build-name="+buildName, "--build-number="+buildNumber)) - require.NoError(t, artifactoryCli.Exec("bp", buildName, buildNumber)) + require.NoError(t, artifactoryCli.Exec("bp", buildName, buildNumber, "--dot-git-path", projectPath)) publishedBuildInfo, found, err := tests.GetBuildInfo(serverDetails, buildName, buildNumber) require.NoError(t, err)