Skip to content

Commit b4efe18

Browse files
eranturgemaneranturgeman
authored
Fix Scan repository failing tests (#1239)
1 parent f0ec429 commit b4efe18

File tree

2 files changed

+93
-70
lines changed

2 files changed

+93
-70
lines changed

‎scanrepository/scanrepository_test.go‎

Lines changed: 69 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"errors"
77
"fmt"
88
services2 "github.com/jfrog/jfrog-client-go/xsc/services"
9+
"io"
910
"net/http"
1011
"net/http/httptest"
1112
"os"
@@ -42,23 +43,6 @@ import (
4243

4344
const rootTestDir = "scanrepository"
4445

45-
var emptyConfigProfile = services2.ConfigProfile{
46-
ProfileName: "test-profile",
47-
GeneralConfig: services2.GeneralConfig{},
48-
FrogbotConfig: services2.FrogbotConfig{
49-
BranchNameTemplate: "",
50-
PrTitleTemplate: "",
51-
CommitMessageTemplate: "",
52-
},
53-
Modules: []services2.Module{
54-
{
55-
ModuleId: 0,
56-
ModuleName: "test-module",
57-
PathFromRoot: ".",
58-
},
59-
},
60-
}
61-
6246
var testPackagesData = []struct {
6347
packageType string
6448
commandName string
@@ -117,22 +101,23 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
117101
expectedMissingFilesInBranch map[string][]string
118102
packageDescriptorPaths []string
119103
aggregateFixes bool
120-
allowPartialResults bool
104+
failUponAnyScannerError bool
121105
}{
122106
{
123107
testName: "aggregate",
124108
expectedPackagesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"uuid", "minimist", "mpath"}},
125-
expectedVersionUpdatesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"^1.2.6", "^9.0.0", "^0.8.4"}},
109+
expectedVersionUpdatesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"1.2.6", "^9.0.0", "0.8.4"}},
126110
packageDescriptorPaths: []string{"package.json"},
127111
aggregateFixes: true,
112+
failUponAnyScannerError: true,
128113
},
129114
{
130115
testName: "aggregate-multi-dir",
131116
expectedPackagesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"uuid", "minimatch", "mpath", "minimist"}},
132-
expectedVersionUpdatesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"^1.2.6", "^9.0.0", "^0.8.4", "^3.0.5"}},
133-
expectedMissingFilesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"npm1/package-lock.json", "npm2/package-lock.json"}},
117+
expectedVersionUpdatesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"1.2.6", "^9.0.0", "0.8.4"}},
134118
packageDescriptorPaths: []string{"npm1/package.json", "npm2/package.json"},
135119
aggregateFixes: true,
120+
failUponAnyScannerError: true,
136121
},
137122
{
138123
testName: "aggregate-no-vul",
@@ -141,22 +126,25 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
141126
expectedVersionUpdatesInBranch: map[string][]string{"master": {}},
142127
packageDescriptorPaths: []string{"package.json"},
143128
aggregateFixes: true,
129+
failUponAnyScannerError: true,
144130
},
145131
{
146132
testName: "aggregate-cant-fix",
147133
// Branch name stays master as no new branch is being created
148134
expectedPackagesInBranch: map[string][]string{"master": {}},
149135
expectedVersionUpdatesInBranch: map[string][]string{"master": {}},
150136
// This is a build tool dependency which should not be fixed.
151-
packageDescriptorPaths: []string{"setup.py"},
152-
aggregateFixes: true,
137+
packageDescriptorPaths: []string{"setup.py"},
138+
aggregateFixes: true,
139+
failUponAnyScannerError: true,
153140
},
154141
{
155142
testName: "non-aggregate",
156143
expectedPackagesInBranch: map[string][]string{"frogbot-minimist-258ad6a538b5ba800f18ae4f6d660302": {"minimist"}},
157-
expectedVersionUpdatesInBranch: map[string][]string{"frogbot-minimist-258ad6a538b5ba800f18ae4f6d660302": {"^1.2.6"}},
144+
expectedVersionUpdatesInBranch: map[string][]string{"frogbot-minimist-258ad6a538b5ba800f18ae4f6d660302": {"1.2.6"}},
158145
packageDescriptorPaths: []string{"package.json"},
159146
aggregateFixes: false,
147+
failUponAnyScannerError: true,
160148
},
161149
{
162150
// This testcase checks the partial results feature. It simulates a failure in the dependency tree construction in the test's project inner module
@@ -165,7 +153,7 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
165153
expectedVersionUpdatesInBranch: map[string][]string{"frogbot-update-68d9dee2475e5986e783d85dfa11baa0-dependencies-master": {"1.2.6", "0.8.4"}},
166154
packageDescriptorPaths: []string{"package.json", "inner-project/package.json"},
167155
aggregateFixes: true,
168-
allowPartialResults: true,
156+
failUponAnyScannerError: false,
169157
},
170158
}
171159
baseDir, err := os.Getwd()
@@ -202,20 +190,20 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
202190
repository, err := utils.BuildRepositoryFromEnv(xrayVersion, xscVersion, client, &gitTestParams, &serverParams, utils.ScanRepository)
203191
assert.NoError(t, err)
204192

205-
// We must set a non-nil config profile to avoid panic
206-
repository.ConfigProfile = &emptyConfigProfile
193+
configProfile := createTestConfigProfile(test.aggregateFixes, test.failUponAnyScannerError)
194+
repository.ConfigProfile = &configProfile
207195

208-
// Run
209196
var cmd = ScanRepositoryCmd{XrayVersion: xrayVersion, XscVersion: xscVersion, dryRun: true, dryRunRepoPath: testDir}
197+
defer func() { assert.NoError(t, os.Chdir(baseDir)) }()
210198
err = cmd.Run(repository, client)
211-
defer func() {
212-
assert.NoError(t, os.Chdir(baseDir))
213-
}()
199+
// Restore CWD immediately so Windows releases the directory handle before the next subtest runs.
200+
assert.NoError(t, os.Chdir(baseDir))
214201

215202
// Validate
203+
repoDir := filepath.Join(testDir, test.testName)
216204
assert.NoError(t, err)
217205
for branch, packages := range test.expectedPackagesInBranch {
218-
resultDiff, err := verifyDependencyFileDiff("master", branch, test.packageDescriptorPaths...)
206+
resultDiff, err := verifyDependencyFileDiff(repoDir, "master", branch, test.packageDescriptorPaths...)
219207
assert.NoError(t, err)
220208
if len(packages) > 0 {
221209
assert.NotEmpty(t, resultDiff)
@@ -231,7 +219,7 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
231219

232220
if len(test.expectedMissingFilesInBranch) > 0 {
233221
for branch, expectedMissingFiles := range test.expectedMissingFilesInBranch {
234-
resultDiff, err := verifyLockFileDiff(branch, expectedMissingFiles...)
222+
resultDiff, err := verifyLockFileDiff(repoDir, branch, expectedMissingFiles...)
235223
assert.NoError(t, err)
236224
assert.Empty(t, resultDiff)
237225
}
@@ -331,15 +319,14 @@ pr body
331319
gitTestParams.Branches = []string{"master"}
332320
repository, err := utils.BuildRepositoryFromEnv(xrayVersion, xscVersion, client, gitTestParams, &serverParams, utils.ScanRepository)
333321
assert.NoError(t, err)
334-
// We must set a non-nil config profile to avoid panic
335-
repository.ConfigProfile = &emptyConfigProfile
322+
configProfile := createTestConfigProfile(true, false)
323+
repository.ConfigProfile = &configProfile
336324

337-
// Run
338325
var cmd = ScanRepositoryCmd{dryRun: true, dryRunRepoPath: testDir}
326+
defer func() { assert.NoError(t, os.Chdir(baseDir)) }()
339327
err = cmd.Run(repository, client)
340-
defer func() {
341-
assert.NoError(t, os.Chdir(baseDir))
342-
}()
328+
// Restore CWD immediately so Windows releases the directory handle before the next subtest runs.
329+
assert.NoError(t, os.Chdir(baseDir))
343330
assert.NoError(t, err)
344331
})
345332
}
@@ -426,11 +413,12 @@ func TestPackageTypeFromScan(t *testing.T) {
426413
for _, file := range files {
427414
log.Info(file)
428415
}
416+
configProfile := createTestConfigProfile(false, false)
429417
scanSetup := utils.ScanDetails{
430418
XrayVersion: xrayVersion,
431419
XscVersion: xscVersion,
432420
ServerDetails: &frogbotParams.Server,
433-
ConfigProfile: &emptyConfigProfile,
421+
ConfigProfile: &configProfile,
434422
}
435423
testScan.scanDetails = &scanSetup
436424
scanResponse, err := testScan.scan()
@@ -779,41 +767,41 @@ func verifyTechnologyNaming(t *testing.T, scanResponse []services.ScanResponse,
779767
}
780768

781769
// Executing git diff to ensure that the intended changes to the dependent file have been made
782-
func verifyDependencyFileDiff(baseBranch string, fixBranch string, packageDescriptorPaths ...string) (output []byte, err error) {
770+
func verifyDependencyFileDiff(repoDir string, baseBranch string, fixBranch string, packageDescriptorPaths ...string) (output []byte, err error) {
783771
log.Debug(fmt.Sprintf("Checking differences in %s between branches %s and %s", packageDescriptorPaths, baseBranch, fixBranch))
784772
// Suppress condition always false warning
785773
//goland:noinspection ALL
786-
var args []string
774+
var cmd *exec.Cmd
787775
if coreutils.IsWindows() {
788-
args = []string{"/c", "git", "diff", baseBranch, fixBranch}
789-
args = append(args, packageDescriptorPaths...)
790-
output, err = exec.Command("cmd", args...).Output()
776+
args := append([]string{"/c", "git", "diff", baseBranch, fixBranch}, packageDescriptorPaths...)
777+
cmd = exec.Command("cmd", args...)
791778
} else {
792-
args = []string{"diff", baseBranch, fixBranch}
793-
args = append(args, packageDescriptorPaths...)
794-
output, err = exec.Command("git", args...).Output()
779+
args := append([]string{"diff", baseBranch, fixBranch}, packageDescriptorPaths...)
780+
cmd = exec.Command("git", args...)
795781
}
782+
cmd.Dir = repoDir
783+
output, err = cmd.Output()
796784
var exitError *exec.ExitError
797785
if errors.As(err, &exitError) {
798786
err = errors.New("git error: " + string(exitError.Stderr))
799787
}
800788
return
801789
}
802790

803-
func verifyLockFileDiff(branchToInspect string, lockFiles ...string) (output []byte, err error) {
791+
func verifyLockFileDiff(repoDir string, branchToInspect string, lockFiles ...string) (output []byte, err error) {
804792
log.Debug(fmt.Sprintf("Checking lock files differences in %s between branches 'master' and '%s'", lockFiles, branchToInspect))
805793
// Suppress condition always false warning
806794
//goland:noinspection ALL
807-
var args []string
795+
var cmd *exec.Cmd
808796
if coreutils.IsWindows() {
809-
args = []string{"/c", "git", "ls-tree", branchToInspect, "--"}
810-
args = append(args, lockFiles...)
811-
output, err = exec.Command("cmd", args...).Output()
797+
args := append([]string{"/c", "git", "ls-tree", branchToInspect, "--"}, lockFiles...)
798+
cmd = exec.Command("cmd", args...)
812799
} else {
813-
args = []string{"ls-tree", branchToInspect, "--"}
814-
args = append(args, lockFiles...)
815-
output, err = exec.Command("git", args...).Output()
800+
args := append([]string{"ls-tree", branchToInspect, "--"}, lockFiles...)
801+
cmd = exec.Command("git", args...)
816802
}
803+
cmd.Dir = repoDir
804+
output, err = cmd.Output()
817805
var exitError *exec.ExitError
818806
if errors.As(err, &exitError) {
819807
err = errors.New("git error: " + string(exitError.Stderr))
@@ -849,7 +837,8 @@ func createScanRepoGitHubHandler(t *testing.T, port *string, response interface{
849837
if r.RequestURI == fmt.Sprintf("/%s", projectName) {
850838
file, err := os.ReadFile(fmt.Sprintf("%s.tar.gz", projectName))
851839
assert.NoError(t, err)
852-
_, err = w.Write(file)
840+
w.Header().Set("Content-Type", "application/octet-stream")
841+
_, err = io.Copy(w, bytes.NewReader(file))
853842
assert.NoError(t, err)
854843
return
855844
}
@@ -892,3 +881,26 @@ func createScanRepoGitHubHandler(t *testing.T, port *string, response interface{
892881
}
893882
}
894883
}
884+
885+
func createTestConfigProfile(aggregateFixes, failUponAnyScannerError bool) services2.ConfigProfile {
886+
return services2.ConfigProfile{
887+
ProfileName: "test-profile",
888+
FrogbotConfig: services2.FrogbotConfig{
889+
CreateAutoFixPr: true,
890+
AggregateFixes: aggregateFixes,
891+
},
892+
GeneralConfig: services2.GeneralConfig{
893+
FailUponAnyScannerError: failUponAnyScannerError,
894+
},
895+
Modules: []services2.Module{{
896+
ModuleId: 0,
897+
ModuleName: "test-module",
898+
PathFromRoot: ".",
899+
ScanConfig: services2.ScanConfig{
900+
ScaScannerConfig: services2.ScaScannerConfig{
901+
EnableScaScan: true,
902+
},
903+
},
904+
}},
905+
}
906+
}

‎utils/testsutils.go‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@ package utils
33
import (
44
"encoding/json"
55
"fmt"
6-
"io"
76
"net/http"
87
"net/http/httptest"
98
"os"
109
"path/filepath"
10+
"runtime"
1111
"strings"
1212
"testing"
1313
"time"
@@ -74,7 +74,20 @@ func CopyTestdataProjectsToTemp(t *testing.T, testDir string) (tmpDir string, re
7474
err = biutils.CopyDir(filepath.Join("..", "testdata", testDir), tmpDir, true, []string{})
7575
assert.NoError(t, err)
7676
restoreFunc = func() {
77-
assert.NoError(t, fileutils.RemoveTempDir(tmpDir))
77+
err := fileutils.RemoveTempDir(tmpDir)
78+
if err == nil {
79+
return
80+
}
81+
// On Windows, directory cleanup can fail due to OS file-system locks held by
82+
// background services (e.g., Windows Defender Real-time Protection, Search Indexer).
83+
// This is a known issue: https://github.com/golang/go/issues/30789
84+
// jfrog-client-go's RemoveTempDir already falls back to RemoveDirContents for this case,
85+
// and CleanOldDirs() will remove the leftover empty directory on the next run.
86+
if runtime.GOOS == "windows" {
87+
t.Logf("Warning: temp dir cleanup failed on Windows (will be retried by CleanOldDirs): %v", err)
88+
return
89+
}
90+
assert.NoError(t, err)
7891
}
7992
return
8093
}
@@ -184,18 +197,22 @@ func CreateXscMockServerForConfigProfile(t *testing.T, xrayVersion string) (mock
184197
updatedContent = strings.Replace(updatedContent, `"path_from_root": "."`, `"path_from_root": "backend"`, 1)
185198
content = []byte(updatedContent)
186199
}
200+
var responseProfile services.ConfigProfile
201+
assert.NoError(t, json.Unmarshal(content, &responseProfile))
202+
w.Header().Set("Content-Type", "application/json")
187203
w.WriteHeader(http.StatusOK)
188-
_, err = w.Write(content)
189-
assert.NoError(t, err)
204+
assert.NoError(t, json.NewEncoder(w).Encode(responseProfile))
190205

191206
// Endpoint to profile by URL
192207
case strings.Contains(r.RequestURI, "/xsc/profile_repos") && isXrayAfterXscMigration:
193208
assert.Equal(t, http.MethodPost, r.Method)
194209
content, err := os.ReadFile("../testdata/configprofile/configProfileExample.json")
195210
assert.NoError(t, err)
211+
var responseProfile services.ConfigProfile
212+
assert.NoError(t, json.Unmarshal(content, &responseProfile))
213+
w.Header().Set("Content-Type", "application/json")
196214
w.WriteHeader(http.StatusOK)
197-
_, err = w.Write(content)
198-
assert.NoError(t, err)
215+
assert.NoError(t, json.NewEncoder(w).Encode(responseProfile))
199216

200217
case r.RequestURI == fmt.Sprintf("/%s/%ssystem/version", apiUrlPart, "xsc"):
201218
_, err := fmt.Fprintf(w, `{"xsc_version": "%s"}`, services.ConfigProfileMinXscVersion)
@@ -232,14 +249,8 @@ func CreateMockServerForDependencySubmission(t *testing.T, owner, repo string) *
232249
}
233250

234251
// Read request body and parse it to ensure all mandatory fields exist
235-
body, err := io.ReadAll(r.Body)
236-
if err != nil {
237-
t.Errorf("Failed to read request body: %v", err)
238-
w.WriteHeader(http.StatusBadRequest)
239-
return
240-
}
241252
var snapshot map[string]interface{}
242-
if err := json.Unmarshal(body, &snapshot); err != nil {
253+
if err := json.NewDecoder(r.Body).Decode(&snapshot); err != nil {
243254
t.Errorf("Failed to parse request body as JSON: %v", err)
244255
w.WriteHeader(http.StatusBadRequest)
245256
return

0 commit comments

Comments
 (0)