From 2cde27c892a03ea4e2ae7aab52ed8c3282a99c45 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Thu, 6 Jun 2024 12:36:32 +0200 Subject: [PATCH 01/32] Moving fill-in of OAuth Client field out of template functions --- config.go | 14 ++++++++++++++ pkg/lib/template/template_loader.go | 12 ++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/config.go b/config.go index 3687d636..72fa4b44 100644 --- a/config.go +++ b/config.go @@ -75,6 +75,9 @@ func NewTestToolConfig(serverURL string, rootDirectory []string, logNetwork bool LogShort: logShort, OAuthClient: Config.Apitest.OAuthClient, } + + config.fillInOAuthClientNames() + err = config.extractTestDirectories() return config, err } @@ -116,3 +119,14 @@ func (config *TestToolConfig) extractTestDirectories() error { } return nil } + +// fillInOAuthClientNames fills in the Client field of loaded OAuthClientConfig +// structs, which the user may have left unset in the config yaml file. +func (config *TestToolConfig) fillInOAuthClientNames() { + for key, clientConfig := range config.OAuthClient { + if clientConfig.Client == "" { + clientConfig.Client = key + config.OAuthClient[key] = clientConfig + } + } +} diff --git a/pkg/lib/template/template_loader.go b/pkg/lib/template/template_loader.go index 9fd41a58..8f31b0a1 100644 --- a/pkg/lib/template/template_loader.go +++ b/pkg/lib/template/template_loader.go @@ -383,7 +383,7 @@ func (loader *Loader) Render( if !ok { return nil, errors.Errorf("OAuth client %q not configured", client) } - oAuthClient.Client = client + return oAuthClient.GetPasswordCredentialsAuthToken(login, password) }, @@ -392,7 +392,7 @@ func (loader *Loader) Render( if !ok { return nil, errors.Errorf("OAuth client %q not configured", client) } - oAuthClient.Client = client + return oAuthClient.GetClientCredentialsAuthToken() }, "oauth2_code_token": func(client string, params ...string) (tok *oauth2.Token, err error) { @@ -400,7 +400,7 @@ func (loader *Loader) Render( if !ok { return nil, errors.Errorf("OAuth client %q not configured", client) } - oAuthClient.Client = client + return oAuthClient.GetCodeAuthToken(params...) }, "oauth2_implicit_token": func(client string, params ...string) (tok *oauth2.Token, err error) { @@ -408,7 +408,7 @@ func (loader *Loader) Render( if !ok { return nil, errors.Errorf("OAuth client %q not configured", client) } - oAuthClient.Client = client + return oAuthClient.GetAuthToken(params...) }, "oauth2_client": func(client string) (c *util.OAuthClientConfig, err error) { @@ -416,7 +416,7 @@ func (loader *Loader) Render( if !ok { return nil, errors.Errorf("OAuth client %s not configured", client) } - oAuthClient.Client = client + return &oAuthClient, nil }, "oauth2_basic_auth": func(client string) (string, error) { @@ -424,7 +424,7 @@ func (loader *Loader) Render( if !ok { return "", errors.Errorf("OAuth client %s not configured", client) } - oAuthClient.Client = client + return "Basic " + base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", oAuthClient.Client, oAuthClient.Secret))), nil }, "query_escape": func(in string) string { From 4c2e4048ba7c283e7484c30780ce22f800dd8c31 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Thu, 6 Jun 2024 12:52:18 +0200 Subject: [PATCH 02/32] datastore: Using RWMutex (also for reading/writing responses) --- pkg/lib/datastore/datastore.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/lib/datastore/datastore.go b/pkg/lib/datastore/datastore.go index f00b16c4..f7453e96 100755 --- a/pkg/lib/datastore/datastore.go +++ b/pkg/lib/datastore/datastore.go @@ -16,7 +16,7 @@ type Datastore struct { storage map[string]any // custom storage responseJson []string // store the responses logDatastore bool - lock *sync.Mutex + lock *sync.RWMutex } func NewStore(logDatastore bool) *Datastore { @@ -24,7 +24,7 @@ func NewStore(logDatastore bool) *Datastore { ds.storage = make(map[string]any, 0) ds.responseJson = make([]string, 0) ds.logDatastore = logDatastore - ds.lock = &sync.Mutex{} + ds.lock = &sync.RWMutex{} return &ds } @@ -84,11 +84,17 @@ func (ds *Datastore) Delete(k string) { // We store the response func (ds *Datastore) UpdateLastResponse(s string) { + ds.lock.Lock() + defer ds.lock.Unlock() + ds.responseJson[len(ds.responseJson)-1] = s } // We store the response func (ds *Datastore) AppendResponse(s string) { + ds.lock.Lock() + defer ds.lock.Unlock() + ds.responseJson = append(ds.responseJson, s) } @@ -167,6 +173,9 @@ func (ds *Datastore) Set(index string, value any) error { } func (ds Datastore) Get(index string) (res any, err error) { + ds.lock.RLock() + defer ds.lock.RUnlock() + // strings are evalulated as int, so // that we can support "-" notations From 4a854bc45dcc3fa386061b9f796f566ec5770b3a Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Thu, 6 Jun 2024 16:39:54 +0200 Subject: [PATCH 03/32] Re-introducing tests for parallel execution, modified for new requirements This partially reverts commit 0a7debf08ed06c2206ca27e61dff80bb0702976a. --- test/parallel/check_collected_responses.json | 26 ++++++++++++++++++++ test/parallel/manifest.json | 21 ++++++++++++++++ test/parallel/parallel.json | 24 ++++++++++++++++++ 3 files changed, 71 insertions(+) create mode 100644 test/parallel/check_collected_responses.json create mode 100644 test/parallel/manifest.json create mode 100644 test/parallel/parallel.json diff --git a/test/parallel/check_collected_responses.json b/test/parallel/check_collected_responses.json new file mode 100644 index 00000000..564d76a6 --- /dev/null +++ b/test/parallel/check_collected_responses.json @@ -0,0 +1,26 @@ +[ + { + "name": "bounce-json: bounce collected responses from N={{datastore "n_parallel"}} parallel runs: {{ datastore "responses" }}", + "request": { + "server_url": "http://localhost{{ datastore "local_port" }}", + "endpoint": "bounce-json", + "method": "POST", + "body": { + "responses": {{ datastore "responses" | marshal }} + } + }, + "response": { + "statuscode": 200, + "body": { + "body": { + "responses": [ + {{ range $idx, $n := N (datastore "n_parallel") }} + {{ if gt $idx 0 }}, {{ end }} + 1 + {{ end }} + ] + } + } + } + } +] \ No newline at end of file diff --git a/test/parallel/manifest.json b/test/parallel/manifest.json new file mode 100644 index 00000000..6c7cfab0 --- /dev/null +++ b/test/parallel/manifest.json @@ -0,0 +1,21 @@ +{{ $local_port := ":9999" }} +{{ $n_parallel := 5 }} +{ + "http_server": { + "addr": "{{ $local_port }}", + "dir": "../_res", + "testmode": false + }, + "name": "parallel run of N={{ $n_parallel }} repetitions", + "tests": [ + { + "name": "port {{ $local_port }}", + "store": { + "n_parallel": {{ $n_parallel }}, + "local_port": {{ $local_port | marshal }} + } + } + , "{{ $n_parallel }}@parallel.json" + , "@check_collected_responses.json" + ] +} diff --git a/test/parallel/parallel.json b/test/parallel/parallel.json new file mode 100644 index 00000000..75b9d71d --- /dev/null +++ b/test/parallel/parallel.json @@ -0,0 +1,24 @@ +[ + { + "name": "bounce-json: bounce n=1", + "request": { + "server_url": "http://localhost{{ datastore "local_port" }}", + "endpoint": "bounce-json", + "method": "POST", + "body": { + "n": 1 + } + }, + "response": { + "statuscode": 200, + "body": { + "body": { + "n": 1 + } + } + }, + "store_response_qjson": { + "responses[]": "body.body.n" + } + } +] \ No newline at end of file From 6d391cc45fc0a4b764256cbf4ff97e3fe79ab206 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Thu, 6 Jun 2024 17:29:23 +0200 Subject: [PATCH 04/32] rewriting old path_utils based on new path syntax With the new, simplified path syntax, we can replace the old path_utils with a simpler, more efficient new version. --- pkg/lib/util/file.go | 11 +-- pkg/lib/util/file_test.go | 127 --------------------------------- pkg/lib/util/path_util.go | 82 ++++++++++++--------- pkg/lib/util/path_util_test.go | 83 +++++++++++++++++++++ 4 files changed, 133 insertions(+), 170 deletions(-) create mode 100644 pkg/lib/util/path_util_test.go diff --git a/pkg/lib/util/file.go b/pkg/lib/util/file.go index d760e7b4..efe99cf3 100644 --- a/pkg/lib/util/file.go +++ b/pkg/lib/util/file.go @@ -17,14 +17,9 @@ var c = &http.Client{ // OpenFileOrUrl opens either a local file or gives the resp.Body from a remote file func OpenFileOrUrl(path, rootDir string) (string, io.ReadCloser, error) { - if strings.HasPrefix(path, "@") { - path = path[1:] - } else if strings.HasPrefix(path, "p@") { - // p@ -> parallel tests - path = path[2:] - } else if IsParallelPathSpec(path) { - // pN@ -> N parallel repetitions of tests - _, path = GetParallelPathSpec(path) + pathSpec, ok := ParsePathSpec(path) + if ok { + path = pathSpec.Path } if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") { diff --git a/pkg/lib/util/file_test.go b/pkg/lib/util/file_test.go index 2bb96001..21f4f333 100644 --- a/pkg/lib/util/file_test.go +++ b/pkg/lib/util/file_test.go @@ -27,133 +27,6 @@ type testOpenFileStruct struct { expHash [16]byte } -func TestGetParallelPathSpec(t *testing.T) { - - tests := []testParallelPathSpecStruct{ - { - pathSpec: "\"", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "[]", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "{}", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "p", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "@", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "1@", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "x@", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "p@", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "@path", - expIsPath: true, - expIsParallel: false, - }, - { - pathSpec: "1@a", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "x@a", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "p1@", - expIsPath: false, - expIsParallel: false, - }, - { - pathSpec: "p1@path", - expIsPath: true, - expIsParallel: true, - expPath: "path", - expParallelRepititions: 1, - }, - { - pathSpec: "p10@path", - expIsPath: true, - expIsParallel: true, - expPath: "path", - expParallelRepititions: 10, - }, - { - pathSpec: "p01@path", - expIsPath: true, - expIsParallel: true, - expPath: "path", - expParallelRepititions: 1, - }, - { - pathSpec: "@path", - expIsPath: true, - expIsParallel: false, - }, - { - pathSpec: "@../path", - expIsPath: true, - expIsParallel: false, - }, - } - - for _, v := range tests { - t.Run(fmt.Sprintf("pathSpec:'%s'", v.pathSpec), func(t *testing.T) { - isPathSpec := IsPathSpec(v.pathSpec) - isParallelPathSpec := IsParallelPathSpec(v.pathSpec) - if isPathSpec != v.expIsPath { - t.Errorf("IsPathSpec: Got %v != %v Exp", isPathSpec, v.expIsPath) - } - if isParallelPathSpec != v.expIsParallel { - t.Errorf("IsParallelPathSpec: Got %v != %v Exp", isParallelPathSpec, v.expIsParallel) - } - - if v.expIsPath { - // the path must also be recognized as a path in case it has trailing quotes - if !IsPathSpec(fmt.Sprintf("\"%s\"", v.pathSpec)) { - t.Errorf("IsPathSpec (with trailing \"): Got %v != %v Exp", isPathSpec, v.expIsPath) - } - } - - if v.expIsParallel { - parallelRepititions, path := GetParallelPathSpec(v.pathSpec) - if parallelRepititions != v.expParallelRepititions { - t.Errorf("ParallelRepititions: Got '%d' != '%d' Exp", parallelRepititions, v.expParallelRepititions) - } - if path != v.expPath { - t.Errorf("Path: Got '%s' != '%s' Exp", path, v.expPath) - } - } - }) - } -} - func TestOpenFileOrUrl(t *testing.T) { filesystem.Fs = afero.NewMemMapFs() ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index 02031598..cc7e469f 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -3,55 +3,67 @@ package util import ( "regexp" "strconv" - "strings" ) /* throughout this file we assume 'manifestDir' to be an absolute path */ -func IsPathSpec(pathSpec string) bool { - if len(pathSpec) < 3 { - return false - } - if strings.HasPrefix(pathSpec, "@") { - return true - } - if strings.HasPrefix(pathSpec, "p@") { - return true - } - // pathSpec could have trailing quotes - if strings.HasPrefix(pathSpec, "\"@") { - return true - } - if strings.HasPrefix(pathSpec, "\"p@") { - return true - } +var pathSpecRegex *regexp.Regexp = regexp.MustCompile(`^([0-9]*)@([^"]+)$`) - return IsParallelPathSpec(pathSpec) -} +// PathSpec is a path specifier for including tests within manifests. +type PathSpec struct { + // Repetitions matches the number of repetitions specified + // in a path spec like "5@foo.json" + Repetitions int -func IsParallelPathSpec(pathSpec string) bool { - n, _ := GetParallelPathSpec(pathSpec) - return n > 0 + // Path matches the literal path, e.g. foo.json in "@foo.json" + Path string } -func GetParallelPathSpec(pathSpec string) (parallelRepititions int, parsedPath string) { - regex := *regexp.MustCompile(`^\"{0,1}p(\d+)@(.+)\"{0,1}$`) - res := regex.FindAllStringSubmatch(pathSpec, -1) +// ParsePathSpec tries to parse the given string into a PathSpec. +// +// It returns a boolean result that indicates if parsing was successful +// (i.e. if s is a valid path specifier). +func ParsePathSpec(s string) (PathSpec, bool) { + // Remove outer quotes, if present + if len(s) >= 2 && s[0] == '"' { + if s[len(s)-1] != '"' { + // path spec must have matching quotes, if quotes are present + return PathSpec{}, false + } - if len(res) != 1 { - return 0, "" + s = s[1 : len(s)-1] } - if len(res[0]) != 3 { - return 0, "" + + // Parse + matches := pathSpecRegex.FindStringSubmatch(s) + if matches == nil { + return PathSpec{}, false + } + + spec := PathSpec{ + Repetitions: 1, + Path: matches[2], // can't be empty, or else it wouldn't match the regex } - parsedPath = res[0][2] - parallelRepititions, err := strconv.Atoi(res[0][1]) - if err != nil { - return 0, parsedPath + // Determine number of repetitions, if supplied + if matches[1] != "" { + reps, err := strconv.Atoi(matches[1]) + if err != nil { + // matches[1] is all digits, so there must be something seriously wrong + panic("error Atoi-ing all-decimal regex match") + } + + spec.Repetitions = reps } - return parallelRepititions, parsedPath + return spec, true +} + +// IsPathSpec is a wrapper around ParsePathSpec that discards the parsed PathSpec. +// It's useful for chaining within boolean expressions. +func IsPathSpec(s string) bool { + _, ok := ParsePathSpec(s) + return ok } diff --git a/pkg/lib/util/path_util_test.go b/pkg/lib/util/path_util_test.go new file mode 100644 index 00000000..ee0bce79 --- /dev/null +++ b/pkg/lib/util/path_util_test.go @@ -0,0 +1,83 @@ +package util + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestParsePathSpec(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + testCases := []struct { + s string + expected PathSpec + }{ + { + s: "@foo.json", + expected: PathSpec{ + Repetitions: 1, + Path: "foo.json", + }, + }, + { + s: "5@bar.json", + expected: PathSpec{ + Repetitions: 5, + Path: "bar.json", + }, + }, + { + s: "123@baz.json", + expected: PathSpec{ + Repetitions: 123, + Path: "baz.json", + }, + }, + } + + for i := range testCases { + testCase := testCases[i] + + t.Run(testCase.s, func(t *testing.T) { + actual, ok := ParsePathSpec(testCase.s) + require.True(t, ok) + require.Equal(t, testCase.expected, actual) + }) + + t.Run(testCase.s+" quoted", func(t *testing.T) { + s := `"` + testCase.s + `"` + + actual, ok := ParsePathSpec(s) + require.True(t, ok) + require.Equal(t, testCase.expected, actual) + }) + } + }) + + t.Run("invalid path specs are detected", func(t *testing.T) { + testCases := []string{ + "", // empty + `"@foo.json`, `@foo.json"`, // superfluous quotes + `foo@bar.baz`, `1.23@foo.json`, // non-digit repetitions + `p@old.syntax`, `p5@old.syntax`, `p123@old.syntax`, // old syntax + } + + for _, testCase := range testCases { + s := testCase + + t.Run(s, func(t *testing.T) { + actual, ok := ParsePathSpec(s) + require.False(t, ok) + require.Zero(t, actual) + }) + + t.Run(s+" quoted", func(t *testing.T) { + sq := `"` + s + `"` + + actual, ok := ParsePathSpec(sq) + require.False(t, ok) + require.Zero(t, actual) + }) + } + }) +} From 261cde53cd74cb140fb6b0e44800b3b078019d8c Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 09:55:39 +0200 Subject: [PATCH 05/32] Re-introducing Parallel Execution, in a simplified version Parallel Execution is re-introduced, but in a much simplified version. p@foo.json / pN@foo.json notation is gone, replaced by N@foo.json. When including a file with N@foo.json, its tests are still run serially, but the entire file is run in N parallel goroutines. --- README.md | 37 ++++++++++++- api_testsuite.go | 131 ++++++++++++++++++++++++++++++++++------------- 2 files changed, 131 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index f083d4a3..0ce9e7ea 100644 --- a/README.md +++ b/README.md @@ -150,7 +150,14 @@ Manifest is loaded as **template**, so you can use variables, Go **range** and * // [SINGLE TESTCASE]: See below for more information // We also support the external loading of a complete test: - "@pathToTest.json" + "@pathToTest.json", + + // By prefixing it with a number, the testtool runs that many instances of + // the included test file in parallel to each other. + // + // Only tests directly included by the manifest are allowed to run in parallel. + // All parallel tests are set to ContinueOnFailure. + "5@pathToTestsThatShouldRunInParallel.json" ] } ``` @@ -413,6 +420,34 @@ However that one will be stripped before parsing the template, which would be ju ** Unlike with delimiters, external tests/requests/responses don't inherit those removals, and need to be specified per file. +## Run tests in parallel +The tool is able to run tests in parallel to themselves. You activate this +mechanism by including an external test file with `N@pathtofile.json`, where N +is the number of parallel "clones" you want to have of the included tests. + +The included tests themselves are still run serially, only the entire set of +tests will run in parallel for the specified number of replications. + +This is useful e.g. for stress-testing an API. + +Only tests directly included by a manifest are allowed to run in parallel. + +**All tests that are run in parallel are implicitly set to ContinueOnFailure, +since otherwise the log/report output would be confusing.** + +```yaml +{ + "name": "Binary Comparison", + "request": { + "endpoint": "suggest", + "method": "GET" + }, + + // Path to binary file with N@ + "response": "123@simple.bin" +} +``` + ## Binary data comparison The tool is able to do a comparison with a binary file. Here we take a MD5 hash of the file and and then later compare diff --git a/api_testsuite.go b/api_testsuite.go index 6d9cbae6..7f8f3116 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -179,8 +179,19 @@ func (ats *Suite) Run() bool { success := true for k, v := range ats.Tests { child := r.NewChild(strconv.Itoa(k)) - sTestSuccess := ats.parseAndRunTest(v, ats.manifestDir, ats.manifestPath, child, ats.loader) + + sTestSuccess := ats.parseAndRunTest( + v, + ats.manifestDir, + ats.manifestPath, + child, + ats.loader, + true, // parallel exec allowed for top-level tests + false, // don't force ContinueOnFail at this point + ) + child.Leave(sTestSuccess) + if !sTestSuccess { success = false break @@ -213,7 +224,10 @@ type TestContainer struct { Path string } -func (ats *Suite) parseAndRunTest(v any, manifestDir, testFilePath string, r *report.ReportElement, rootLoader template.Loader) bool { +func (ats *Suite) parseAndRunTest( + v any, manifestDir, testFilePath string, r *report.ReportElement, + rootLoader template.Loader, allowParallelExec bool, forceContinueOnFail bool, +) bool { //Init variables // logrus.Warnf("Test %s, Prev delimiters: %#v", testFilePath, rootLoader.Delimiters) loader := template.NewLoader(ats.datastore) @@ -227,7 +241,28 @@ func (ats *Suite) parseAndRunTest(v any, manifestDir, testFilePath string, r *re loader.ServerURL = serverURL loader.OAuthClient = ats.Config.OAuthClient - //Get the Manifest with @ logic + // Determine number of parallel repetitions, if any + repetitions := 1 + if vStr, ok := v.(string); ok { + pathSpec, ok := util.ParsePathSpec(vStr) + if ok { + repetitions = pathSpec.Repetitions + } + } + + // Configure parallel repetitions, if specified + if repetitions > 1 { + // Check that parallel repetitions are actually allowed + if !allowParallelExec { + logrus.Error(fmt.Errorf("parallel repetitions are not allowed in nested tests (%s)", testFilePath)) + return false + } + + // If we're running in parallel repetition mode, force subordinate tests to ContinueOnFail + forceContinueOnFail = true + } + + // Get the Manifest with @ logic fileh, testObj, err := template.LoadManifestDataAsRawJson(v, manifestDir) dir := filepath.Dir(fileh) if fileh != "" { @@ -265,37 +300,55 @@ func (ats *Suite) parseAndRunTest(v any, manifestDir, testFilePath string, r *re } // Execute test cases - for i, testCase := range testCases { - var success bool - - // If testCase can be unmarshalled as string, we may have a - // reference to another test using @ notation at hand - var testCaseStr string - err = util.Unmarshal(testCase, &testCaseStr) - if err == nil && util.IsPathSpec(testCaseStr) { - // Recurse if the testCase points to another file using @ notation - success = ats.parseAndRunTest( - testCaseStr, - filepath.Join(manifestDir, dir), - testFilePath, - r, - loader, - ) - } else { - // Otherwise simply run the literal test case - success = ats.runLiteralTest( - TestContainer{ - CaseByte: testCase, - Path: filepath.Join(manifestDir, dir), - }, - r, - testFilePath, - loader, - i, - ) - } - - if !success { + successCh := make(chan bool, repetitions) + + for repeatIdx := range repetitions { + go func() { + for testIdx, testCase := range testCases { + var success bool + + // If testCase can be unmarshalled as string, we may have a + // reference to another test using @ notation at hand + var testCaseStr string + err = util.Unmarshal(testCase, &testCaseStr) + if err == nil && util.IsPathSpec(testCaseStr) { + // Recurse if the testCase points to another file using @ notation + success = ats.parseAndRunTest( + testCaseStr, + filepath.Join(manifestDir, dir), + testFilePath, + r, + loader, + false, // no parallel exec allowed in nested tests + forceContinueOnFail, + ) + } else { + // Otherwise simply run the literal test case + success = ats.runLiteralTest( + TestContainer{ + CaseByte: testCase, + Path: filepath.Join(manifestDir, dir), + }, + r, + testFilePath, + loader, + repeatIdx*len(testCases)+testIdx, + forceContinueOnFail, + ) + } + + if !success { + successCh <- false + return + } + } + + successCh <- true + }() + } + + for range repetitions { + if success := <-successCh; !success { return false } } @@ -303,7 +356,10 @@ func (ats *Suite) parseAndRunTest(v any, manifestDir, testFilePath string, r *re return true } -func (ats *Suite) runLiteralTest(tc TestContainer, r *report.ReportElement, testFilePath string, loader template.Loader, k int) bool { +func (ats *Suite) runLiteralTest( + tc TestContainer, r *report.ReportElement, testFilePath string, loader template.Loader, + index int, forceContinueOnFailure bool, +) bool { r.SetName(testFilePath) var test Case @@ -320,10 +376,13 @@ func (ats *Suite) runLiteralTest(tc TestContainer, r *report.ReportElement, test test.loader = loader test.manifestDir = tc.Path test.suiteIndex = ats.index - test.index = k + test.index = index test.dataStore = ats.datastore test.standardHeader = ats.StandardHeader test.standardHeaderFromStore = ats.StandardHeaderFromStore + if forceContinueOnFailure { + test.ContinueOnFailure = true + } if test.LogNetwork == nil { test.LogNetwork = &ats.Config.LogNetwork } From feba700b7e12a9ebb44c7e6b486d2fef085546e9 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 11:44:11 +0200 Subject: [PATCH 06/32] Parallel Exec: No longer forcing ContinueOnFailure --- README.md | 7 ++----- api_testsuite.go | 15 +++------------ 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 0ce9e7ea..2f2e691d 100644 --- a/README.md +++ b/README.md @@ -156,7 +156,6 @@ Manifest is loaded as **template**, so you can use variables, Go **range** and * // the included test file in parallel to each other. // // Only tests directly included by the manifest are allowed to run in parallel. - // All parallel tests are set to ContinueOnFailure. "5@pathToTestsThatShouldRunInParallel.json" ] } @@ -167,8 +166,9 @@ Manifest is loaded as **template**, so you can use variables, Go **range** and * ### manifest.json ```yaml { - // Define if the testuite should continue even if this test fails. (default:false) + // Define if the test suite should continue even if this test fails. (default: false) "continue_on_failure": true, + // Name to identify this single test. Is important for the log. Try to give an explaning name "name": "Testname", @@ -432,9 +432,6 @@ This is useful e.g. for stress-testing an API. Only tests directly included by a manifest are allowed to run in parallel. -**All tests that are run in parallel are implicitly set to ContinueOnFailure, -since otherwise the log/report output would be confusing.** - ```yaml { "name": "Binary Comparison", diff --git a/api_testsuite.go b/api_testsuite.go index 7f8f3116..2e9b5ca9 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -186,8 +186,7 @@ func (ats *Suite) Run() bool { ats.manifestPath, child, ats.loader, - true, // parallel exec allowed for top-level tests - false, // don't force ContinueOnFail at this point + true, // parallel exec allowed for top-level tests ) child.Leave(sTestSuccess) @@ -226,7 +225,7 @@ type TestContainer struct { func (ats *Suite) parseAndRunTest( v any, manifestDir, testFilePath string, r *report.ReportElement, - rootLoader template.Loader, allowParallelExec bool, forceContinueOnFail bool, + rootLoader template.Loader, allowParallelExec bool, ) bool { //Init variables // logrus.Warnf("Test %s, Prev delimiters: %#v", testFilePath, rootLoader.Delimiters) @@ -257,9 +256,6 @@ func (ats *Suite) parseAndRunTest( logrus.Error(fmt.Errorf("parallel repetitions are not allowed in nested tests (%s)", testFilePath)) return false } - - // If we're running in parallel repetition mode, force subordinate tests to ContinueOnFail - forceContinueOnFail = true } // Get the Manifest with @ logic @@ -320,7 +316,6 @@ func (ats *Suite) parseAndRunTest( r, loader, false, // no parallel exec allowed in nested tests - forceContinueOnFail, ) } else { // Otherwise simply run the literal test case @@ -333,7 +328,6 @@ func (ats *Suite) parseAndRunTest( testFilePath, loader, repeatIdx*len(testCases)+testIdx, - forceContinueOnFail, ) } @@ -358,7 +352,7 @@ func (ats *Suite) parseAndRunTest( func (ats *Suite) runLiteralTest( tc TestContainer, r *report.ReportElement, testFilePath string, loader template.Loader, - index int, forceContinueOnFailure bool, + index int, ) bool { r.SetName(testFilePath) @@ -380,9 +374,6 @@ func (ats *Suite) runLiteralTest( test.dataStore = ats.datastore test.standardHeader = ats.StandardHeader test.standardHeaderFromStore = ats.StandardHeaderFromStore - if forceContinueOnFailure { - test.ContinueOnFailure = true - } if test.LogNetwork == nil { test.LogNetwork = &ats.Config.LogNetwork } From b84c527025e0d71f19e12a4c9faec83601c1f7ee Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 11:51:22 +0200 Subject: [PATCH 07/32] Parallel Exec: Working with WaitGroup and atomic.Uint32 instead of channels --- api_testsuite.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index 2e9b5ca9..731fa606 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -9,6 +9,8 @@ import ( "os" "path/filepath" "strconv" + "sync" + "sync/atomic" "time" "github.com/pkg/errors" @@ -296,10 +298,15 @@ func (ats *Suite) parseAndRunTest( } // Execute test cases - successCh := make(chan bool, repetitions) + var successCount atomic.Uint32 + var waitGroup sync.WaitGroup + + waitGroup.Add(repetitions) for repeatIdx := range repetitions { go func() { + defer waitGroup.Done() + for testIdx, testCase := range testCases { var success bool @@ -332,22 +339,18 @@ func (ats *Suite) parseAndRunTest( } if !success { - successCh <- false + // note that successCount is not incremented return } } - successCh <- true + successCount.Add(1) }() } - for range repetitions { - if success := <-successCh; !success { - return false - } - } + waitGroup.Wait() - return true + return successCount.Load() == uint32(repetitions) } func (ats *Suite) runLiteralTest( From 8496fc4f2f975c899c2cdbdbd80032ce1838a386 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 12:00:17 +0200 Subject: [PATCH 08/32] wording: repetitions -> parallel runs --- api_testsuite.go | 20 ++++++++++---------- pkg/lib/util/path_util.go | 14 +++++++------- pkg/lib/util/path_util_test.go | 14 +++++++------- test/parallel/manifest.json | 2 +- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index 731fa606..61f6ef45 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -242,20 +242,20 @@ func (ats *Suite) parseAndRunTest( loader.ServerURL = serverURL loader.OAuthClient = ats.Config.OAuthClient - // Determine number of parallel repetitions, if any - repetitions := 1 + // Determine number of parallel runs + parallelRuns := 1 if vStr, ok := v.(string); ok { pathSpec, ok := util.ParsePathSpec(vStr) if ok { - repetitions = pathSpec.Repetitions + parallelRuns = pathSpec.ParallelRuns } } - // Configure parallel repetitions, if specified - if repetitions > 1 { - // Check that parallel repetitions are actually allowed + // Configure parallel runs, if specified + if parallelRuns > 1 { + // Check that parallel runs are actually allowed if !allowParallelExec { - logrus.Error(fmt.Errorf("parallel repetitions are not allowed in nested tests (%s)", testFilePath)) + logrus.Error(fmt.Errorf("parallel runs are not allowed in nested tests (%s)", testFilePath)) return false } } @@ -301,9 +301,9 @@ func (ats *Suite) parseAndRunTest( var successCount atomic.Uint32 var waitGroup sync.WaitGroup - waitGroup.Add(repetitions) + waitGroup.Add(parallelRuns) - for repeatIdx := range repetitions { + for repeatIdx := range parallelRuns { go func() { defer waitGroup.Done() @@ -350,7 +350,7 @@ func (ats *Suite) parseAndRunTest( waitGroup.Wait() - return successCount.Load() == uint32(repetitions) + return successCount.Load() == uint32(parallelRuns) } func (ats *Suite) runLiteralTest( diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index cc7e469f..c4b962a0 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -13,9 +13,9 @@ var pathSpecRegex *regexp.Regexp = regexp.MustCompile(`^([0-9]*)@([^"]+)$`) // PathSpec is a path specifier for including tests within manifests. type PathSpec struct { - // Repetitions matches the number of repetitions specified + // ParallelRuns matches the number of parallel runs specified // in a path spec like "5@foo.json" - Repetitions int + ParallelRuns int // Path matches the literal path, e.g. foo.json in "@foo.json" Path string @@ -43,19 +43,19 @@ func ParsePathSpec(s string) (PathSpec, bool) { } spec := PathSpec{ - Repetitions: 1, - Path: matches[2], // can't be empty, or else it wouldn't match the regex + ParallelRuns: 1, + Path: matches[2], // can't be empty, or else it wouldn't match the regex } - // Determine number of repetitions, if supplied + // Determine number of parallel runs, if supplied if matches[1] != "" { - reps, err := strconv.Atoi(matches[1]) + prs, err := strconv.Atoi(matches[1]) if err != nil { // matches[1] is all digits, so there must be something seriously wrong panic("error Atoi-ing all-decimal regex match") } - spec.Repetitions = reps + spec.ParallelRuns = prs } return spec, true diff --git a/pkg/lib/util/path_util_test.go b/pkg/lib/util/path_util_test.go index ee0bce79..2c70c7e6 100644 --- a/pkg/lib/util/path_util_test.go +++ b/pkg/lib/util/path_util_test.go @@ -15,22 +15,22 @@ func TestParsePathSpec(t *testing.T) { { s: "@foo.json", expected: PathSpec{ - Repetitions: 1, - Path: "foo.json", + ParallelRuns: 1, + Path: "foo.json", }, }, { s: "5@bar.json", expected: PathSpec{ - Repetitions: 5, - Path: "bar.json", + ParallelRuns: 5, + Path: "bar.json", }, }, { s: "123@baz.json", expected: PathSpec{ - Repetitions: 123, - Path: "baz.json", + ParallelRuns: 123, + Path: "baz.json", }, }, } @@ -58,7 +58,7 @@ func TestParsePathSpec(t *testing.T) { testCases := []string{ "", // empty `"@foo.json`, `@foo.json"`, // superfluous quotes - `foo@bar.baz`, `1.23@foo.json`, // non-digit repetitions + `foo@bar.baz`, `1.23@foo.json`, // non-digit parallel runs `p@old.syntax`, `p5@old.syntax`, `p123@old.syntax`, // old syntax } diff --git a/test/parallel/manifest.json b/test/parallel/manifest.json index 6c7cfab0..78c8ad73 100644 --- a/test/parallel/manifest.json +++ b/test/parallel/manifest.json @@ -6,7 +6,7 @@ "dir": "../_res", "testmode": false }, - "name": "parallel run of N={{ $n_parallel }} repetitions", + "name": "parallel run of N={{ $n_parallel }} parallel runs", "tests": [ { "name": "port {{ $local_port }}", From 61d734effdea0a8cdc1934322bd9dc4fccf84917 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 12:05:03 +0200 Subject: [PATCH 09/32] file_test: Removing unused struct left over from moving out path_util tests --- pkg/lib/util/file_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/lib/util/file_test.go b/pkg/lib/util/file_test.go index 21f4f333..72763805 100644 --- a/pkg/lib/util/file_test.go +++ b/pkg/lib/util/file_test.go @@ -13,14 +13,6 @@ import ( "github.com/spf13/afero" ) -type testParallelPathSpecStruct struct { - pathSpec string - expIsPath bool - expIsParallel bool - expPath string - expParallelRepititions int -} - type testOpenFileStruct struct { filename string expError error From d3517d9a8150d9f63445b572bf3ceb89a58c0a23 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 12:10:03 +0200 Subject: [PATCH 10/32] path_util: Removing outdated comment, added explanation for pathSpecRegex --- pkg/lib/util/path_util.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index c4b962a0..f2693ec2 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -5,10 +5,8 @@ import ( "strconv" ) -/* -throughout this file we assume 'manifestDir' to be an absolute path -*/ - +// pathSpecRegex validates and (via its capture groups) breaks up a +// path spec string in a single step (see ParsePathSpec). var pathSpecRegex *regexp.Regexp = regexp.MustCompile(`^([0-9]*)@([^"]+)$`) // PathSpec is a path specifier for including tests within manifests. From 9413bffa7d5babaaf7b31706077096f50930bae6 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 12:26:49 +0200 Subject: [PATCH 11/32] path_util: Removing support for quoted strings --- pkg/lib/util/path_util.go | 13 +------------ pkg/lib/util/path_util_test.go | 19 +------------------ 2 files changed, 2 insertions(+), 30 deletions(-) diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index f2693ec2..f9f00694 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -7,7 +7,7 @@ import ( // pathSpecRegex validates and (via its capture groups) breaks up a // path spec string in a single step (see ParsePathSpec). -var pathSpecRegex *regexp.Regexp = regexp.MustCompile(`^([0-9]*)@([^"]+)$`) +var pathSpecRegex *regexp.Regexp = regexp.MustCompile(`^([0-9]*)@(.+)$`) // PathSpec is a path specifier for including tests within manifests. type PathSpec struct { @@ -24,17 +24,6 @@ type PathSpec struct { // It returns a boolean result that indicates if parsing was successful // (i.e. if s is a valid path specifier). func ParsePathSpec(s string) (PathSpec, bool) { - // Remove outer quotes, if present - if len(s) >= 2 && s[0] == '"' { - if s[len(s)-1] != '"' { - // path spec must have matching quotes, if quotes are present - return PathSpec{}, false - } - - s = s[1 : len(s)-1] - } - - // Parse matches := pathSpecRegex.FindStringSubmatch(s) if matches == nil { return PathSpec{}, false diff --git a/pkg/lib/util/path_util_test.go b/pkg/lib/util/path_util_test.go index 2c70c7e6..5518b315 100644 --- a/pkg/lib/util/path_util_test.go +++ b/pkg/lib/util/path_util_test.go @@ -43,21 +43,12 @@ func TestParsePathSpec(t *testing.T) { require.True(t, ok) require.Equal(t, testCase.expected, actual) }) - - t.Run(testCase.s+" quoted", func(t *testing.T) { - s := `"` + testCase.s + `"` - - actual, ok := ParsePathSpec(s) - require.True(t, ok) - require.Equal(t, testCase.expected, actual) - }) } }) t.Run("invalid path specs are detected", func(t *testing.T) { testCases := []string{ - "", // empty - `"@foo.json`, `@foo.json"`, // superfluous quotes + "", // empty `foo@bar.baz`, `1.23@foo.json`, // non-digit parallel runs `p@old.syntax`, `p5@old.syntax`, `p123@old.syntax`, // old syntax } @@ -70,14 +61,6 @@ func TestParsePathSpec(t *testing.T) { require.False(t, ok) require.Zero(t, actual) }) - - t.Run(s+" quoted", func(t *testing.T) { - sq := `"` + s + `"` - - actual, ok := ParsePathSpec(sq) - require.False(t, ok) - require.Zero(t, actual) - }) } }) } From b8e281b436e91b797ab6b62e1716684baa24955d Mon Sep 17 00:00:00 2001 From: Martin Rode Date: Fri, 7 Jun 2024 13:04:50 +0200 Subject: [PATCH 12/32] Simplified ParsePathSpec to not use a regexp (more readable now) see #72658 --- pkg/lib/util/path_util.go | 39 ++++++-------------- test/parallel/check_collected_responses.json | 4 +- test/parallel/parallel.json | 2 +- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index f9f00694..46e726a6 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -1,14 +1,10 @@ package util import ( - "regexp" "strconv" + "strings" ) -// pathSpecRegex validates and (via its capture groups) breaks up a -// path spec string in a single step (see ParsePathSpec). -var pathSpecRegex *regexp.Regexp = regexp.MustCompile(`^([0-9]*)@(.+)$`) - // PathSpec is a path specifier for including tests within manifests. type PathSpec struct { // ParallelRuns matches the number of parallel runs specified @@ -21,30 +17,19 @@ type PathSpec struct { // ParsePathSpec tries to parse the given string into a PathSpec. // -// It returns a boolean result that indicates if parsing was successful -// (i.e. if s is a valid path specifier). -func ParsePathSpec(s string) (PathSpec, bool) { - matches := pathSpecRegex.FindStringSubmatch(s) - if matches == nil { - return PathSpec{}, false - } - - spec := PathSpec{ - ParallelRuns: 1, - Path: matches[2], // can't be empty, or else it wouldn't match the regex +// It returns a boolean result that indicates if parsing was successful (i.e. if +// s is a valid path specifier). The string takes the format "[n]@file.json". +func ParsePathSpec(s string) (spec PathSpec, ok bool) { + var parallelRuns string + parallelRuns, spec.Path, ok = strings.Cut(s, "@") + if parallelRuns != "" { + spec.ParallelRuns, _ = strconv.Atoi(parallelRuns) + } else { + spec.ParallelRuns = 1 } - - // Determine number of parallel runs, if supplied - if matches[1] != "" { - prs, err := strconv.Atoi(matches[1]) - if err != nil { - // matches[1] is all digits, so there must be something seriously wrong - panic("error Atoi-ing all-decimal regex match") - } - - spec.ParallelRuns = prs + if !ok || spec.Path == "" || spec.ParallelRuns <= 0 { + return PathSpec{}, false } - return spec, true } diff --git a/test/parallel/check_collected_responses.json b/test/parallel/check_collected_responses.json index 564d76a6..1eceb324 100644 --- a/test/parallel/check_collected_responses.json +++ b/test/parallel/check_collected_responses.json @@ -1,8 +1,8 @@ [ { - "name": "bounce-json: bounce collected responses from N={{datastore "n_parallel"}} parallel runs: {{ datastore "responses" }}", + "name": "bounce-json: bounce collected responses from N={{datastore `n_parallel` }} parallel runs: {{ datastore `responses` }}", "request": { - "server_url": "http://localhost{{ datastore "local_port" }}", + "server_url": "http://localhost{{ datastore `local_port` }}", "endpoint": "bounce-json", "method": "POST", "body": { diff --git a/test/parallel/parallel.json b/test/parallel/parallel.json index 75b9d71d..d07a68ff 100644 --- a/test/parallel/parallel.json +++ b/test/parallel/parallel.json @@ -2,7 +2,7 @@ { "name": "bounce-json: bounce n=1", "request": { - "server_url": "http://localhost{{ datastore "local_port" }}", + "server_url": "http://localhost{{ datastore `local_port` }}", "endpoint": "bounce-json", "method": "POST", "body": { From ee474068286d7a5ed4f8705b504f834247aa08f2 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 13:20:45 +0200 Subject: [PATCH 13/32] path_util: Adding test cases for zero/negative parallel runs --- pkg/lib/util/path_util_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/lib/util/path_util_test.go b/pkg/lib/util/path_util_test.go index 5518b315..534db842 100644 --- a/pkg/lib/util/path_util_test.go +++ b/pkg/lib/util/path_util_test.go @@ -49,8 +49,9 @@ func TestParsePathSpec(t *testing.T) { t.Run("invalid path specs are detected", func(t *testing.T) { testCases := []string{ "", // empty - `foo@bar.baz`, `1.23@foo.json`, // non-digit parallel runs - `p@old.syntax`, `p5@old.syntax`, `p123@old.syntax`, // old syntax + "foo@bar.baz", "1.23@foo.json", // non-digit parallel runs + "p@old.syntax", "p5@old.syntax", "p123@old.syntax", // old syntax + "0@foo.json", "-1@foo.json", "-123@foo.json", // zero or negative parallel runs } for _, testCase := range testCases { From 982a0265f39cbca023c2be722b596e6dc86ede45 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 13:34:23 +0200 Subject: [PATCH 14/32] path_util: Changing ParsePathSpec to return error instead of bool --- api_testsuite.go | 11 +++++++---- pkg/lib/util/file.go | 4 ++-- pkg/lib/util/path_util.go | 24 ++++++++++++++++-------- pkg/lib/util/path_util_test.go | 8 ++++---- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index 61f6ef45..50945ff3 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -242,13 +242,16 @@ func (ats *Suite) parseAndRunTest( loader.ServerURL = serverURL loader.OAuthClient = ats.Config.OAuthClient - // Determine number of parallel runs + // Parse PathSpec (if any) and determine number of parallel runs parallelRuns := 1 if vStr, ok := v.(string); ok { - pathSpec, ok := util.ParsePathSpec(vStr) - if ok { - parallelRuns = pathSpec.ParallelRuns + pathSpec, err := util.ParsePathSpec(vStr) + if err != nil { + logrus.Error(fmt.Errorf("test string is not a valid path spec: %w", err)) + return false } + + parallelRuns = pathSpec.ParallelRuns } // Configure parallel runs, if specified diff --git a/pkg/lib/util/file.go b/pkg/lib/util/file.go index efe99cf3..864807e0 100644 --- a/pkg/lib/util/file.go +++ b/pkg/lib/util/file.go @@ -17,8 +17,8 @@ var c = &http.Client{ // OpenFileOrUrl opens either a local file or gives the resp.Body from a remote file func OpenFileOrUrl(path, rootDir string) (string, io.ReadCloser, error) { - pathSpec, ok := ParsePathSpec(path) - if ok { + pathSpec, err := ParsePathSpec(path) + if err == nil { path = pathSpec.Path } diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index 46e726a6..5e3f8d64 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -1,6 +1,7 @@ package util import ( + "fmt" "strconv" "strings" ) @@ -17,25 +18,32 @@ type PathSpec struct { // ParsePathSpec tries to parse the given string into a PathSpec. // -// It returns a boolean result that indicates if parsing was successful (i.e. if -// s is a valid path specifier). The string takes the format "[n]@file.json". -func ParsePathSpec(s string) (spec PathSpec, ok bool) { +// The string takes the format "[n]@file.json". Invalid path specs +// result in an error. +func ParsePathSpec(s string) (spec PathSpec, err error) { + var ok bool var parallelRuns string + parallelRuns, spec.Path, ok = strings.Cut(s, "@") if parallelRuns != "" { - spec.ParallelRuns, _ = strconv.Atoi(parallelRuns) + spec.ParallelRuns, err = strconv.Atoi(parallelRuns) + if err != nil { + return PathSpec{}, fmt.Errorf("error parsing ParallelRuns of path spec %q: %w", s, err) + } } else { spec.ParallelRuns = 1 } + if !ok || spec.Path == "" || spec.ParallelRuns <= 0 { - return PathSpec{}, false + return PathSpec{}, fmt.Errorf("invalid path spec %q", s) } - return spec, true + + return spec, err } // IsPathSpec is a wrapper around ParsePathSpec that discards the parsed PathSpec. // It's useful for chaining within boolean expressions. func IsPathSpec(s string) bool { - _, ok := ParsePathSpec(s) - return ok + _, err := ParsePathSpec(s) + return err == nil } diff --git a/pkg/lib/util/path_util_test.go b/pkg/lib/util/path_util_test.go index 534db842..a399b966 100644 --- a/pkg/lib/util/path_util_test.go +++ b/pkg/lib/util/path_util_test.go @@ -39,8 +39,8 @@ func TestParsePathSpec(t *testing.T) { testCase := testCases[i] t.Run(testCase.s, func(t *testing.T) { - actual, ok := ParsePathSpec(testCase.s) - require.True(t, ok) + actual, err := ParsePathSpec(testCase.s) + require.NoError(t, err) require.Equal(t, testCase.expected, actual) }) } @@ -58,8 +58,8 @@ func TestParsePathSpec(t *testing.T) { s := testCase t.Run(s, func(t *testing.T) { - actual, ok := ParsePathSpec(s) - require.False(t, ok) + actual, err := ParsePathSpec(s) + require.Error(t, err) require.Zero(t, actual) }) } From 33bed31babe5f7cfce157f74272c4ac72fa2f301 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 13:39:27 +0200 Subject: [PATCH 15/32] path_util: Allowing ParallelRuns=0 --- pkg/lib/util/path_util.go | 2 +- pkg/lib/util/path_util_test.go | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index 5e3f8d64..1fc57f32 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -34,7 +34,7 @@ func ParsePathSpec(s string) (spec PathSpec, err error) { spec.ParallelRuns = 1 } - if !ok || spec.Path == "" || spec.ParallelRuns <= 0 { + if !ok || spec.Path == "" || spec.ParallelRuns < 0 { return PathSpec{}, fmt.Errorf("invalid path spec %q", s) } diff --git a/pkg/lib/util/path_util_test.go b/pkg/lib/util/path_util_test.go index a399b966..4097db4b 100644 --- a/pkg/lib/util/path_util_test.go +++ b/pkg/lib/util/path_util_test.go @@ -33,6 +33,13 @@ func TestParsePathSpec(t *testing.T) { Path: "baz.json", }, }, + { + s: "0@foobar.json", + expected: PathSpec{ + ParallelRuns: 0, + Path: "foobar.json", + }, + }, } for i := range testCases { @@ -51,7 +58,7 @@ func TestParsePathSpec(t *testing.T) { "", // empty "foo@bar.baz", "1.23@foo.json", // non-digit parallel runs "p@old.syntax", "p5@old.syntax", "p123@old.syntax", // old syntax - "0@foo.json", "-1@foo.json", "-123@foo.json", // zero or negative parallel runs + "-1@foo.json", "-123@foo.json", // negative parallel runs } for _, testCase := range testCases { From 40bba7a3f7b7514d7524b08b96f5484b98eeebf8 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 13:46:41 +0200 Subject: [PATCH 16/32] api_testsuite: naming repeatIdx -> runIdx --- api_testsuite.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index 50945ff3..70ad671a 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -306,7 +306,7 @@ func (ats *Suite) parseAndRunTest( waitGroup.Add(parallelRuns) - for repeatIdx := range parallelRuns { + for runIdx := range parallelRuns { go func() { defer waitGroup.Done() @@ -337,7 +337,7 @@ func (ats *Suite) parseAndRunTest( r, testFilePath, loader, - repeatIdx*len(testCases)+testIdx, + runIdx*len(testCases)+testIdx, ) } From 259dd1759f41e7ec302b9607d8a88e6ac0f03e9d Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 14:31:25 +0200 Subject: [PATCH 17/32] Adding parallel_run_idx template function --- api_testsuite.go | 42 ++++++++++++-------- pkg/lib/template/template_loader.go | 8 ++++ test/parallel/check_collected_responses.json | 4 +- test/parallel/parallel.json | 25 +----------- test/parallel/parallel_case.json | 24 +++++++++++ 5 files changed, 61 insertions(+), 42 deletions(-) create mode 100644 test/parallel/parallel_case.json diff --git a/api_testsuite.go b/api_testsuite.go index 70ad671a..3cfca05b 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -47,7 +47,7 @@ type Suite struct { manifestPath string reporterRoot *report.ReportElement index int - serverURL string + serverURL *url.URL httpServer http.Server httpServerProxy *httpproxy.Proxy httpServerDir string @@ -151,6 +151,12 @@ func NewTestSuite(config TestToolConfig, manifestPath string, manifestDir string //Append suite manifest path to name, so we know in an automatic setup where the test is loaded from suite.Name = fmt.Sprintf("%s (%s)", suite.Name, manifestPath) + // Parse serverURL + suite.serverURL, err = url.Parse(suite.Config.ServerURL) + if err != nil { + return nil, fmt.Errorf("can not load server url : %s", err) + } + // init store err = suite.datastore.SetMap(suite.Store) if err != nil { @@ -187,7 +193,7 @@ func (ats *Suite) Run() bool { ats.manifestDir, ats.manifestPath, child, - ats.loader, + ats.buildLoader(ats.loader, -1), true, // parallel exec allowed for top-level tests ) @@ -225,23 +231,21 @@ type TestContainer struct { Path string } -func (ats *Suite) parseAndRunTest( - v any, manifestDir, testFilePath string, r *report.ReportElement, - rootLoader template.Loader, allowParallelExec bool, -) bool { - //Init variables - // logrus.Warnf("Test %s, Prev delimiters: %#v", testFilePath, rootLoader.Delimiters) +func (ats *Suite) buildLoader(rootLoader template.Loader, parallelRunIdx int) template.Loader { loader := template.NewLoader(ats.datastore) loader.Delimiters = rootLoader.Delimiters loader.HTTPServerHost = ats.HTTPServerHost - serverURL, err := url.Parse(ats.Config.ServerURL) - if err != nil { - logrus.Error(fmt.Errorf("can not load server url into test (%s): %s", testFilePath, err)) - return false - } - loader.ServerURL = serverURL + loader.ServerURL = ats.serverURL loader.OAuthClient = ats.Config.OAuthClient + loader.ParallelRunIdx = parallelRunIdx + return loader +} + +func (ats *Suite) parseAndRunTest( + v any, manifestDir, testFilePath string, r *report.ReportElement, + loader template.Loader, allowParallelExec bool, +) bool { // Parse PathSpec (if any) and determine number of parallel runs parallelRuns := 1 if vStr, ok := v.(string); ok { @@ -307,12 +311,16 @@ func (ats *Suite) parseAndRunTest( waitGroup.Add(parallelRuns) for runIdx := range parallelRuns { + runIdxCapture := runIdx + go func() { defer waitGroup.Done() for testIdx, testCase := range testCases { var success bool + caseLoader := ats.buildLoader(loader, runIdxCapture) + // If testCase can be unmarshalled as string, we may have a // reference to another test using @ notation at hand var testCaseStr string @@ -324,7 +332,7 @@ func (ats *Suite) parseAndRunTest( filepath.Join(manifestDir, dir), testFilePath, r, - loader, + caseLoader, false, // no parallel exec allowed in nested tests ) } else { @@ -336,8 +344,8 @@ func (ats *Suite) parseAndRunTest( }, r, testFilePath, - loader, - runIdx*len(testCases)+testIdx, + caseLoader, + runIdxCapture*len(testCases)+testIdx, ) } diff --git a/pkg/lib/template/template_loader.go b/pkg/lib/template/template_loader.go index 8f31b0a1..0b376a8e 100644 --- a/pkg/lib/template/template_loader.go +++ b/pkg/lib/template/template_loader.go @@ -49,6 +49,9 @@ type Loader struct { ServerURL *url.URL OAuthClient util.OAuthClientsConfig Delimiters delimiters + + // ParallelRunIdx is the index of the Parallel Run that this Loader is used in + ParallelRunIdx int } func NewLoader(datastore *datastore.Datastore) Loader { @@ -489,6 +492,11 @@ func (loader *Loader) Render( q := u.Query() return q.Get(qKey) }, + // parallel_run_idx returns the index of the Parallel Run that the current template + // is rendered in. + "parallel_run_idx": func() int { + return loader.ParallelRunIdx + }, } tmpl, err := template. New("tmpl"). diff --git a/test/parallel/check_collected_responses.json b/test/parallel/check_collected_responses.json index 1eceb324..7b2b6240 100644 --- a/test/parallel/check_collected_responses.json +++ b/test/parallel/check_collected_responses.json @@ -16,11 +16,11 @@ "responses": [ {{ range $idx, $n := N (datastore "n_parallel") }} {{ if gt $idx 0 }}, {{ end }} - 1 + {{ $idx }} {{ end }} ] } } } } -] \ No newline at end of file +] diff --git a/test/parallel/parallel.json b/test/parallel/parallel.json index d07a68ff..ed5ad4db 100644 --- a/test/parallel/parallel.json +++ b/test/parallel/parallel.json @@ -1,24 +1,3 @@ [ - { - "name": "bounce-json: bounce n=1", - "request": { - "server_url": "http://localhost{{ datastore `local_port` }}", - "endpoint": "bounce-json", - "method": "POST", - "body": { - "n": 1 - } - }, - "response": { - "statuscode": 200, - "body": { - "body": { - "n": 1 - } - } - }, - "store_response_qjson": { - "responses[]": "body.body.n" - } - } -] \ No newline at end of file + "@parallel_case.json" +] diff --git a/test/parallel/parallel_case.json b/test/parallel/parallel_case.json new file mode 100644 index 00000000..9f00e470 --- /dev/null +++ b/test/parallel/parallel_case.json @@ -0,0 +1,24 @@ +[ + { + "name": "bounce-json: bounce n=1", + "request": { + "server_url": "http://localhost{{ datastore `local_port` }}", + "endpoint": "bounce-json", + "method": "POST", + "body": { + "n": {{ parallel_run_idx }} + } + }, + "response": { + "statuscode": 200, + "body": { + "body": { + "n": {{ parallel_run_idx }} + } + } + }, + "store_response_qjson": { + "responses[]": "body.body.n" + } + } +] From 7f69f522aa5c45d199d60d1c253fdb55b6ad3ca5 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 14:43:53 +0200 Subject: [PATCH 18/32] Adding README section about parallel_run_idx template function --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 2f2e691d..bf35cca7 100644 --- a/README.md +++ b/README.md @@ -2293,6 +2293,10 @@ Removes from **key** from **url**'s query, returns the **url** with the **key** Returns the **value** from the **url**'s query for **key**. In case of an error, an empty string is returned. Unparsable urls are ignored and an empty string is returned. +## `parallel_run_idx` +Returns the index of the Parallel Run that the template is executed in, or -1 if it is not executed +within a parallel run. + # HTTP Server The apitest tool includes an HTTP Server. It can be used to serve files from the local disk temporarily. The HTTP Server can run in test mode. In this mode, the apitest tool does not run any tests, but starts the HTTP Server in the foreground, until CTRL-C in pressed. From eee784270bef68f7af2a9b09071fb243230df53f Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 16:16:29 +0200 Subject: [PATCH 19/32] Moving template rendering into Goroutine + fixes to parallel_run_idx --- api_testsuite.go | 84 ++++++++++--------- pkg/lib/template/template_loader.go | 2 +- .../check_collected_responses.json | 0 test/parallel/{ => direct}/manifest.json | 0 .../parallel.json} | 2 +- .../indirect/check_collected_responses.json | 26 ++++++ test/parallel/indirect/manifest.json | 21 +++++ test/parallel/indirect/parallel.json | 1 + test/parallel/indirect/parallel_case.json | 22 +++++ test/parallel/indirect/parallel_indirect.json | 1 + test/parallel/parallel.json | 3 - 11 files changed, 119 insertions(+), 43 deletions(-) rename test/parallel/{ => direct}/check_collected_responses.json (100%) rename test/parallel/{ => direct}/manifest.json (100%) rename test/parallel/{parallel_case.json => direct/parallel.json} (89%) create mode 100644 test/parallel/indirect/check_collected_responses.json create mode 100644 test/parallel/indirect/manifest.json create mode 100644 test/parallel/indirect/parallel.json create mode 100644 test/parallel/indirect/parallel_case.json create mode 100644 test/parallel/indirect/parallel_indirect.json delete mode 100644 test/parallel/parallel.json diff --git a/api_testsuite.go b/api_testsuite.go index 3cfca05b..dc62d2c8 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -193,7 +193,7 @@ func (ats *Suite) Run() bool { ats.manifestDir, ats.manifestPath, child, - ats.buildLoader(ats.loader, -1), + ats.loader, true, // parallel exec allowed for top-level tests ) @@ -237,14 +237,19 @@ func (ats *Suite) buildLoader(rootLoader template.Loader, parallelRunIdx int) te loader.HTTPServerHost = ats.HTTPServerHost loader.ServerURL = ats.serverURL loader.OAuthClient = ats.Config.OAuthClient - loader.ParallelRunIdx = parallelRunIdx + + if rootLoader.ParallelRunIdx < 0 { + loader.ParallelRunIdx = parallelRunIdx + } else { + loader.ParallelRunIdx = rootLoader.ParallelRunIdx + } return loader } func (ats *Suite) parseAndRunTest( v any, manifestDir, testFilePath string, r *report.ReportElement, - loader template.Loader, allowParallelExec bool, + rootLoader template.Loader, allowParallelExec bool, ) bool { // Parse PathSpec (if any) and determine number of parallel runs parallelRuns := 1 @@ -268,7 +273,7 @@ func (ats *Suite) parseAndRunTest( } // Get the Manifest with @ logic - fileh, testObj, err := template.LoadManifestDataAsRawJson(v, manifestDir) + fileh, testRaw, err := template.LoadManifestDataAsRawJson(v, manifestDir) dir := filepath.Dir(fileh) if fileh != "" { testFilePath = filepath.Join(filepath.Dir(testFilePath), fileh) @@ -279,31 +284,6 @@ func (ats *Suite) parseAndRunTest( return false } - // Parse as template always - testObj, err = loader.Render(testObj, filepath.Join(manifestDir, dir), nil) - if err != nil { - r.SaveToReportLog(err.Error()) - logrus.Error(fmt.Errorf("can not render template (%s): %s", testFilePath, err)) - return false - } - - // Build list of test cases - var testCases []json.RawMessage - err = util.Unmarshal(testObj, &testCases) - if err != nil { - // Input could not be deserialized into list, try to deserialize into single object - var singleTest json.RawMessage - err = util.Unmarshal(testObj, &singleTest) - if err != nil { - // Malformed json - r.SaveToReportLog(err.Error()) - logrus.Error(fmt.Errorf("can not unmarshal (%s): %s", testFilePath, err)) - return false - } - - testCases = []json.RawMessage{singleTest} - } - // Execute test cases var successCount atomic.Uint32 var waitGroup sync.WaitGroup @@ -311,16 +291,44 @@ func (ats *Suite) parseAndRunTest( waitGroup.Add(parallelRuns) for runIdx := range parallelRuns { - runIdxCapture := runIdx - - go func() { + go func(runIdx int) { defer waitGroup.Done() + // Build template loader + loader := ats.buildLoader(rootLoader, runIdx) + + // Parse as template always + testRendered, err := loader.Render(testRaw, filepath.Join(manifestDir, dir), nil) + if err != nil { + r.SaveToReportLog(err.Error()) + logrus.Error(fmt.Errorf("can not render template (%s): %s", testFilePath, err)) + + // note that successCount is not incremented + return + } + + // Build list of test cases + var testCases []json.RawMessage + err = util.Unmarshal(testRendered, &testCases) + if err != nil { + // Input could not be deserialized into list, try to deserialize into single object + var singleTest json.RawMessage + err = util.Unmarshal(testRendered, &singleTest) + if err != nil { + // Malformed json + r.SaveToReportLog(err.Error()) + logrus.Error(fmt.Errorf("can not unmarshal (%s): %s", testFilePath, err)) + + // note that successCount is not incremented + return + } + + testCases = []json.RawMessage{singleTest} + } + for testIdx, testCase := range testCases { var success bool - caseLoader := ats.buildLoader(loader, runIdxCapture) - // If testCase can be unmarshalled as string, we may have a // reference to another test using @ notation at hand var testCaseStr string @@ -332,7 +340,7 @@ func (ats *Suite) parseAndRunTest( filepath.Join(manifestDir, dir), testFilePath, r, - caseLoader, + loader, false, // no parallel exec allowed in nested tests ) } else { @@ -344,8 +352,8 @@ func (ats *Suite) parseAndRunTest( }, r, testFilePath, - caseLoader, - runIdxCapture*len(testCases)+testIdx, + loader, + runIdx*len(testCases)+testIdx, ) } @@ -356,7 +364,7 @@ func (ats *Suite) parseAndRunTest( } successCount.Add(1) - }() + }(runIdx) } waitGroup.Wait() diff --git a/pkg/lib/template/template_loader.go b/pkg/lib/template/template_loader.go index 0b376a8e..d3a57d78 100644 --- a/pkg/lib/template/template_loader.go +++ b/pkg/lib/template/template_loader.go @@ -55,7 +55,7 @@ type Loader struct { } func NewLoader(datastore *datastore.Datastore) Loader { - return Loader{datastore: datastore} + return Loader{datastore: datastore, ParallelRunIdx: -1} } // Render loads and executes a manifest template. diff --git a/test/parallel/check_collected_responses.json b/test/parallel/direct/check_collected_responses.json similarity index 100% rename from test/parallel/check_collected_responses.json rename to test/parallel/direct/check_collected_responses.json diff --git a/test/parallel/manifest.json b/test/parallel/direct/manifest.json similarity index 100% rename from test/parallel/manifest.json rename to test/parallel/direct/manifest.json diff --git a/test/parallel/parallel_case.json b/test/parallel/direct/parallel.json similarity index 89% rename from test/parallel/parallel_case.json rename to test/parallel/direct/parallel.json index 9f00e470..9097c081 100644 --- a/test/parallel/parallel_case.json +++ b/test/parallel/direct/parallel.json @@ -1,6 +1,6 @@ [ { - "name": "bounce-json: bounce n=1", + "name": "bounce-json: bounce n={{ parallel_run_idx }}", "request": { "server_url": "http://localhost{{ datastore `local_port` }}", "endpoint": "bounce-json", diff --git a/test/parallel/indirect/check_collected_responses.json b/test/parallel/indirect/check_collected_responses.json new file mode 100644 index 00000000..7b2b6240 --- /dev/null +++ b/test/parallel/indirect/check_collected_responses.json @@ -0,0 +1,26 @@ +[ + { + "name": "bounce-json: bounce collected responses from N={{datastore `n_parallel` }} parallel runs: {{ datastore `responses` }}", + "request": { + "server_url": "http://localhost{{ datastore `local_port` }}", + "endpoint": "bounce-json", + "method": "POST", + "body": { + "responses": {{ datastore "responses" | marshal }} + } + }, + "response": { + "statuscode": 200, + "body": { + "body": { + "responses": [ + {{ range $idx, $n := N (datastore "n_parallel") }} + {{ if gt $idx 0 }}, {{ end }} + {{ $idx }} + {{ end }} + ] + } + } + } + } +] diff --git a/test/parallel/indirect/manifest.json b/test/parallel/indirect/manifest.json new file mode 100644 index 00000000..95a37b9c --- /dev/null +++ b/test/parallel/indirect/manifest.json @@ -0,0 +1,21 @@ +{{ $local_port := ":9999" }} +{{ $n_parallel := 5 }} +{ + "http_server": { + "addr": "{{ $local_port }}", + "dir": "../_res", + "testmode": false + }, + "name": "parallel run of N={{ $n_parallel }} parallel runs (indirect)", + "tests": [ + { + "name": "port {{ $local_port }}", + "store": { + "n_parallel": {{ $n_parallel }}, + "local_port": {{ $local_port | marshal }} + } + } + , "{{ $n_parallel }}@parallel.json" + , "@check_collected_responses.json" + ] +} diff --git a/test/parallel/indirect/parallel.json b/test/parallel/indirect/parallel.json new file mode 100644 index 00000000..9ad95ca9 --- /dev/null +++ b/test/parallel/indirect/parallel.json @@ -0,0 +1 @@ +"@parallel_indirect.json" diff --git a/test/parallel/indirect/parallel_case.json b/test/parallel/indirect/parallel_case.json new file mode 100644 index 00000000..9a8ce7c6 --- /dev/null +++ b/test/parallel/indirect/parallel_case.json @@ -0,0 +1,22 @@ +{ + "name": "bounce-json: bounce n={{ parallel_run_idx }}", + "request": { + "server_url": "http://localhost{{ datastore `local_port` }}", + "endpoint": "bounce-json", + "method": "POST", + "body": { + "n": {{ parallel_run_idx }} + } + }, + "response": { + "statuscode": 200, + "body": { + "body": { + "n": {{ parallel_run_idx }} + } + } + }, + "store_response_qjson": { + "responses[]": "body.body.n" + } +} diff --git a/test/parallel/indirect/parallel_indirect.json b/test/parallel/indirect/parallel_indirect.json new file mode 100644 index 00000000..eef091a6 --- /dev/null +++ b/test/parallel/indirect/parallel_indirect.json @@ -0,0 +1 @@ +"@parallel_case.json" diff --git a/test/parallel/parallel.json b/test/parallel/parallel.json deleted file mode 100644 index ed5ad4db..00000000 --- a/test/parallel/parallel.json +++ /dev/null @@ -1,3 +0,0 @@ -[ - "@parallel_case.json" -] From 0154ef87d69341972d743314416884488354dd00 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 16:36:13 +0200 Subject: [PATCH 20/32] Moving test goroutine into separate function for readability --- api_testsuite.go | 157 +++++++++++++++++++++++++---------------------- 1 file changed, 83 insertions(+), 74 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index dc62d2c8..bd8d73bc 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -291,80 +291,10 @@ func (ats *Suite) parseAndRunTest( waitGroup.Add(parallelRuns) for runIdx := range parallelRuns { - go func(runIdx int) { - defer waitGroup.Done() - - // Build template loader - loader := ats.buildLoader(rootLoader, runIdx) - - // Parse as template always - testRendered, err := loader.Render(testRaw, filepath.Join(manifestDir, dir), nil) - if err != nil { - r.SaveToReportLog(err.Error()) - logrus.Error(fmt.Errorf("can not render template (%s): %s", testFilePath, err)) - - // note that successCount is not incremented - return - } - - // Build list of test cases - var testCases []json.RawMessage - err = util.Unmarshal(testRendered, &testCases) - if err != nil { - // Input could not be deserialized into list, try to deserialize into single object - var singleTest json.RawMessage - err = util.Unmarshal(testRendered, &singleTest) - if err != nil { - // Malformed json - r.SaveToReportLog(err.Error()) - logrus.Error(fmt.Errorf("can not unmarshal (%s): %s", testFilePath, err)) - - // note that successCount is not incremented - return - } - - testCases = []json.RawMessage{singleTest} - } - - for testIdx, testCase := range testCases { - var success bool - - // If testCase can be unmarshalled as string, we may have a - // reference to another test using @ notation at hand - var testCaseStr string - err = util.Unmarshal(testCase, &testCaseStr) - if err == nil && util.IsPathSpec(testCaseStr) { - // Recurse if the testCase points to another file using @ notation - success = ats.parseAndRunTest( - testCaseStr, - filepath.Join(manifestDir, dir), - testFilePath, - r, - loader, - false, // no parallel exec allowed in nested tests - ) - } else { - // Otherwise simply run the literal test case - success = ats.runLiteralTest( - TestContainer{ - CaseByte: testCase, - Path: filepath.Join(manifestDir, dir), - }, - r, - testFilePath, - loader, - runIdx*len(testCases)+testIdx, - ) - } - - if !success { - // note that successCount is not incremented - return - } - } - - successCount.Add(1) - }(runIdx) + go ats.testGoroutine( + &waitGroup, &successCount, manifestDir, testFilePath, r, rootLoader, + runIdx, testRaw, dir, + ) } waitGroup.Wait() @@ -372,6 +302,85 @@ func (ats *Suite) parseAndRunTest( return successCount.Load() == uint32(parallelRuns) } +func (ats *Suite) testGoroutine( + waitGroup *sync.WaitGroup, successCount *atomic.Uint32, + manifestDir, testFilePath string, r *report.ReportElement, rootLoader template.Loader, + runIdx int, testRaw json.RawMessage, dir string, // TODO: refactor paths / dirs (including DRY below at filepath.Join(mainfestDir, dir) +) { + defer waitGroup.Done() + + // Build template loader + loader := ats.buildLoader(rootLoader, runIdx) + + // Parse testRaw as template + testRendered, err := loader.Render(testRaw, filepath.Join(manifestDir, dir), nil) + if err != nil { + r.SaveToReportLog(err.Error()) + logrus.Error(fmt.Errorf("can not render template (%s): %s", testFilePath, err)) + + // note that successCount is not incremented + return + } + + // Build list of test cases + var testCases []json.RawMessage + err = util.Unmarshal(testRendered, &testCases) + if err != nil { + // Input could not be deserialized into list, try to deserialize into single object + var singleTest json.RawMessage + err = util.Unmarshal(testRendered, &singleTest) + if err != nil { + // Malformed json + r.SaveToReportLog(err.Error()) + logrus.Error(fmt.Errorf("can not unmarshal (%s): %s", testFilePath, err)) + + // note that successCount is not incremented + return + } + + testCases = []json.RawMessage{singleTest} + } + + for testIdx, testCase := range testCases { + var success bool + + // If testCase can be unmarshalled as string, we may have a + // reference to another test using @ notation at hand + var testCaseStr string + err = util.Unmarshal(testCase, &testCaseStr) + if err == nil && util.IsPathSpec(testCaseStr) { + // Recurse if the testCase points to another file using @ notation + success = ats.parseAndRunTest( + testCaseStr, + filepath.Join(manifestDir, dir), + testFilePath, + r, + loader, + false, // no parallel exec allowed in nested tests + ) + } else { + // Otherwise simply run the literal test case + success = ats.runLiteralTest( + TestContainer{ + CaseByte: testCase, + Path: filepath.Join(manifestDir, dir), + }, + r, + testFilePath, + loader, + runIdx*len(testCases)+testIdx, + ) + } + + if !success { + // note that successCount is not incremented + return + } + } + + successCount.Add(1) +} + func (ats *Suite) runLiteralTest( tc TestContainer, r *report.ReportElement, testFilePath string, loader template.Loader, index int, From f56b29382619a952f7ea64dddced291774ac68eb Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 16:38:16 +0200 Subject: [PATCH 21/32] combining redundant if statements --- api_testsuite.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index bd8d73bc..8305da84 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -263,13 +263,10 @@ func (ats *Suite) parseAndRunTest( parallelRuns = pathSpec.ParallelRuns } - // Configure parallel runs, if specified - if parallelRuns > 1 { - // Check that parallel runs are actually allowed - if !allowParallelExec { - logrus.Error(fmt.Errorf("parallel runs are not allowed in nested tests (%s)", testFilePath)) - return false - } + // If parallel runs are requested, check that they're actually allowed + if parallelRuns > 1 && !allowParallelExec { + logrus.Error(fmt.Errorf("parallel runs are not allowed in nested tests (%s)", testFilePath)) + return false } // Get the Manifest with @ logic From d2e2f3b0ccde55e35639b5dbb2d0b1dde9da1b4c Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 16:52:10 +0200 Subject: [PATCH 22/32] parseAndRunTest / testGoroutine: Simplifying testFilePath / manifestDir --- api_testsuite.go | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index 8305da84..28e6b933 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -190,7 +190,6 @@ func (ats *Suite) Run() bool { sTestSuccess := ats.parseAndRunTest( v, - ats.manifestDir, ats.manifestPath, child, ats.loader, @@ -248,8 +247,8 @@ func (ats *Suite) buildLoader(rootLoader template.Loader, parallelRunIdx int) te } func (ats *Suite) parseAndRunTest( - v any, manifestDir, testFilePath string, r *report.ReportElement, - rootLoader template.Loader, allowParallelExec bool, + v any, testFilePath string, r *report.ReportElement, rootLoader template.Loader, + allowParallelExec bool, ) bool { // Parse PathSpec (if any) and determine number of parallel runs parallelRuns := 1 @@ -270,10 +269,9 @@ func (ats *Suite) parseAndRunTest( } // Get the Manifest with @ logic - fileh, testRaw, err := template.LoadManifestDataAsRawJson(v, manifestDir) - dir := filepath.Dir(fileh) - if fileh != "" { - testFilePath = filepath.Join(filepath.Dir(testFilePath), fileh) + referencedFilePath, testRaw, err := template.LoadManifestDataAsRawJson(v, filepath.Dir(testFilePath)) + if referencedFilePath != "" { + testFilePath = filepath.Join(filepath.Dir(testFilePath), referencedFilePath) } if err != nil { r.SaveToReportLog(err.Error()) @@ -289,8 +287,8 @@ func (ats *Suite) parseAndRunTest( for runIdx := range parallelRuns { go ats.testGoroutine( - &waitGroup, &successCount, manifestDir, testFilePath, r, rootLoader, - runIdx, testRaw, dir, + &waitGroup, &successCount, testFilePath, r, rootLoader, + runIdx, testRaw, ) } @@ -301,16 +299,18 @@ func (ats *Suite) parseAndRunTest( func (ats *Suite) testGoroutine( waitGroup *sync.WaitGroup, successCount *atomic.Uint32, - manifestDir, testFilePath string, r *report.ReportElement, rootLoader template.Loader, - runIdx int, testRaw json.RawMessage, dir string, // TODO: refactor paths / dirs (including DRY below at filepath.Join(mainfestDir, dir) + testFilePath string, r *report.ReportElement, rootLoader template.Loader, + runIdx int, testRaw json.RawMessage, ) { defer waitGroup.Done() + testFileDir := filepath.Dir(testFilePath) + // Build template loader loader := ats.buildLoader(rootLoader, runIdx) // Parse testRaw as template - testRendered, err := loader.Render(testRaw, filepath.Join(manifestDir, dir), nil) + testRendered, err := loader.Render(testRaw, testFileDir, nil) if err != nil { r.SaveToReportLog(err.Error()) logrus.Error(fmt.Errorf("can not render template (%s): %s", testFilePath, err)) @@ -349,7 +349,6 @@ func (ats *Suite) testGoroutine( // Recurse if the testCase points to another file using @ notation success = ats.parseAndRunTest( testCaseStr, - filepath.Join(manifestDir, dir), testFilePath, r, loader, @@ -360,7 +359,7 @@ func (ats *Suite) testGoroutine( success = ats.runLiteralTest( TestContainer{ CaseByte: testCase, - Path: filepath.Join(manifestDir, dir), + Path: testFileDir, }, r, testFilePath, From a51fab21bd05a5e68ff0a3e7b12178ae927071f7 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 16:59:22 +0200 Subject: [PATCH 23/32] ParsePathSpec: Returning *PathSpec instead of PathSpec --- pkg/lib/util/path_util.go | 10 ++++++---- pkg/lib/util/path_util_test.go | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index 1fc57f32..310ae3b3 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -20,25 +20,27 @@ type PathSpec struct { // // The string takes the format "[n]@file.json". Invalid path specs // result in an error. -func ParsePathSpec(s string) (spec PathSpec, err error) { +func ParsePathSpec(s string) (*PathSpec, error) { var ok bool + var err error var parallelRuns string + var spec PathSpec parallelRuns, spec.Path, ok = strings.Cut(s, "@") if parallelRuns != "" { spec.ParallelRuns, err = strconv.Atoi(parallelRuns) if err != nil { - return PathSpec{}, fmt.Errorf("error parsing ParallelRuns of path spec %q: %w", s, err) + return nil, fmt.Errorf("error parsing ParallelRuns of path spec %q: %w", s, err) } } else { spec.ParallelRuns = 1 } if !ok || spec.Path == "" || spec.ParallelRuns < 0 { - return PathSpec{}, fmt.Errorf("invalid path spec %q", s) + return nil, fmt.Errorf("invalid path spec %q", s) } - return spec, err + return &spec, err } // IsPathSpec is a wrapper around ParsePathSpec that discards the parsed PathSpec. diff --git a/pkg/lib/util/path_util_test.go b/pkg/lib/util/path_util_test.go index 4097db4b..32bfc762 100644 --- a/pkg/lib/util/path_util_test.go +++ b/pkg/lib/util/path_util_test.go @@ -48,7 +48,7 @@ func TestParsePathSpec(t *testing.T) { t.Run(testCase.s, func(t *testing.T) { actual, err := ParsePathSpec(testCase.s) require.NoError(t, err) - require.Equal(t, testCase.expected, actual) + require.Equal(t, testCase.expected, *actual) }) } }) @@ -67,7 +67,7 @@ func TestParsePathSpec(t *testing.T) { t.Run(s, func(t *testing.T) { actual, err := ParsePathSpec(s) require.Error(t, err) - require.Zero(t, actual) + require.Nil(t, actual) }) } }) From 8fa716382e173734ff70645a9208fce3ecef860d Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Fri, 7 Jun 2024 17:02:09 +0200 Subject: [PATCH 24/32] Removing tabs in test JSONs --- test/parallel/direct/check_collected_responses.json | 2 +- test/parallel/indirect/check_collected_responses.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/direct/check_collected_responses.json b/test/parallel/direct/check_collected_responses.json index 7b2b6240..1712aaab 100644 --- a/test/parallel/direct/check_collected_responses.json +++ b/test/parallel/direct/check_collected_responses.json @@ -16,7 +16,7 @@ "responses": [ {{ range $idx, $n := N (datastore "n_parallel") }} {{ if gt $idx 0 }}, {{ end }} - {{ $idx }} + {{ $idx }} {{ end }} ] } diff --git a/test/parallel/indirect/check_collected_responses.json b/test/parallel/indirect/check_collected_responses.json index 7b2b6240..1712aaab 100644 --- a/test/parallel/indirect/check_collected_responses.json +++ b/test/parallel/indirect/check_collected_responses.json @@ -16,7 +16,7 @@ "responses": [ {{ range $idx, $n := N (datastore "n_parallel") }} {{ if gt $idx 0 }}, {{ end }} - {{ $idx }} + {{ $idx }} {{ end }} ] } From beb5c4044a28125c41f54963f730aad289333d43 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Mon, 17 Jun 2024 13:46:53 +0200 Subject: [PATCH 25/32] OpenFileOrUrl: Clarifying purpose and error handling of ParsePathSpec call --- pkg/lib/util/file.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/lib/util/file.go b/pkg/lib/util/file.go index 864807e0..6c8c75fb 100644 --- a/pkg/lib/util/file.go +++ b/pkg/lib/util/file.go @@ -17,6 +17,10 @@ var c = &http.Client{ // OpenFileOrUrl opens either a local file or gives the resp.Body from a remote file func OpenFileOrUrl(path, rootDir string) (string, io.ReadCloser, error) { + // Note that err is not propagated here since we're only checking + // *if* path is an @-Notation PathSpec. If so, we unwrap its path. + // If not, we assume we don't have an @-Notation at hand in the first place. + // Validation of PathSpec is performed further up in calling code. pathSpec, err := ParsePathSpec(path) if err == nil { path = pathSpec.Path From e9649557656866daeb0ef5decc880022c717f8ea Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Mon, 17 Jun 2024 13:48:25 +0200 Subject: [PATCH 26/32] ParsePathSpec: Using var block --- pkg/lib/util/path_util.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index 310ae3b3..a925e22f 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -21,10 +21,12 @@ type PathSpec struct { // The string takes the format "[n]@file.json". Invalid path specs // result in an error. func ParsePathSpec(s string) (*PathSpec, error) { - var ok bool - var err error - var parallelRuns string - var spec PathSpec + var ( + ok bool + err error + parallelRuns string + spec PathSpec + ) parallelRuns, spec.Path, ok = strings.Cut(s, "@") if parallelRuns != "" { From eafc0ee20c716e89a8287ed375c9ac9fb2838bdc Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Mon, 17 Jun 2024 16:24:57 +0200 Subject: [PATCH 27/32] OpenFileOrUrl: Moving PathSpec parsing to caller side --- pkg/lib/api/build_policies.go | 26 +++++++++++++++----------- pkg/lib/template/template_funcs.go | 7 ++++--- pkg/lib/template/template_loader.go | 8 +------- pkg/lib/template/util.go | 15 ++++++++------- pkg/lib/util/file.go | 15 +++------------ pkg/lib/util/file_test.go | 16 ++++++++++------ test/proxy/read_from_proxies.json | 4 ++-- 7 files changed, 43 insertions(+), 48 deletions(-) diff --git a/pkg/lib/api/build_policies.go b/pkg/lib/api/build_policies.go index d60d977e..7af90c86 100644 --- a/pkg/lib/api/build_policies.go +++ b/pkg/lib/api/build_policies.go @@ -31,30 +31,28 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body additionalHeaders["Content-Type"] = w.FormDataContentType() for key, val := range request.Body.(map[string]any) { - if key == "file:filename" { continue } - pathSpec, ok := val.(util.JsonString) + rawPathSpec, ok := val.(util.JsonString) if !ok { return additionalHeaders, body, fmt.Errorf("pathSpec should be a string") } - if !util.IsPathSpec(pathSpec) { - return additionalHeaders, body, fmt.Errorf("pathSpec %s is not valid", pathSpec) + pathSpec, err := util.ParsePathSpec(rawPathSpec) + if err != nil { + return additionalHeaders, body, fmt.Errorf("pathSpec %s is not valid: %w", rawPathSpec, err) } - var err error - - _, file, err := util.OpenFileOrUrl(pathSpec, request.ManifestDir) + file, err := util.OpenFileOrUrl(pathSpec.Path, request.ManifestDir) if err != nil { return additionalHeaders, nil, err } - defer file.Close() + defer file.Close() // FIXME: defer in for var part io.Writer if replaceFilename == nil { - part, err = w.CreateFormFile(key, pathSpec[1:]) + part, err = w.CreateFormFile(key, pathSpec.Path) } else { part, err = w.CreateFormFile(key, *replaceFilename) } @@ -107,17 +105,23 @@ func buildRegular(request Request) (additionalHeaders map[string]string, body io } func buildFile(req Request) (map[string]string, io.Reader, error) { - headers := map[string]string{} if req.BodyFile == "" { return nil, nil, errors.New(`Request.buildFile: Missing "body_file"`) } - _, file, err := util.OpenFileOrUrl(req.BodyFile, req.ManifestDir) + path := req.BodyFile + pathSpec, err := util.ParsePathSpec(req.BodyFile) + if err == nil && pathSpec != nil { // we unwrap the path, if an @-notation path spec was passed into body_file + path = pathSpec.Path + } + + file, err := util.OpenFileOrUrl(path, req.ManifestDir) if err != nil { return nil, nil, err } + // FIXME: file is left open AND passed out of function wihout ReadCloser! return headers, file, err } diff --git a/pkg/lib/template/template_funcs.go b/pkg/lib/template/template_funcs.go index 1eb046c5..833f0d00 100644 --- a/pkg/lib/template/template_funcs.go +++ b/pkg/lib/template/template_funcs.go @@ -3,7 +3,7 @@ package template import ( "encoding/json" "fmt" - "io/ioutil" + "io" "path/filepath" "reflect" "strconv" @@ -323,12 +323,13 @@ func divide(b, a any) (any, error) { } func fileReadInternal(pathOrURL, rootDir string) ([]byte, error) { - _, file, err := util.OpenFileOrUrl(pathOrURL, rootDir) + file, err := util.OpenFileOrUrl(pathOrURL, rootDir) if err != nil { return nil, errors.Wrapf(err, "fileReadInternal: %q", pathOrURL) } + defer file.Close() - data, err := ioutil.ReadAll(file) + data, err := io.ReadAll(file) if err != nil { return nil, errors.Wrapf(err, "fileReadInternal: %q", pathOrURL) } diff --git a/pkg/lib/template/template_loader.go b/pkg/lib/template/template_loader.go index d3a57d78..b85b279e 100644 --- a/pkg/lib/template/template_loader.go +++ b/pkg/lib/template/template_loader.go @@ -23,7 +23,6 @@ import ( "github.com/programmfabrik/apitest/pkg/lib/util" - "io/ioutil" "path/filepath" _ "github.com/mattn/go-sqlite3" @@ -114,12 +113,7 @@ func (loader *Loader) Render( return strings.Split(s, sep) }, "md5sum": func(path string) (string, error) { - _, file, err := util.OpenFileOrUrl(path, rootDir) - if err != nil { - return "", err - } - - fileBytes, err := ioutil.ReadAll(file) + fileBytes, err := fileReadInternal(path, rootDir) if err != nil { return "", err } diff --git a/pkg/lib/template/util.go b/pkg/lib/template/util.go index 9b914da5..6ca75cf6 100644 --- a/pkg/lib/template/util.go +++ b/pkg/lib/template/util.go @@ -3,29 +3,30 @@ package template import ( "encoding/json" "fmt" - "io/ioutil" + "io" "github.com/programmfabrik/apitest/pkg/lib/util" ) -func loadFileFromPathSpec(pathSpec, manifestDir string) (string, []byte, error) { - if !util.IsPathSpec(pathSpec) { - return "", nil, fmt.Errorf("spec was expected to be path spec, got %s instead", pathSpec) +func loadFileFromPathSpec(rawPathSpec, manifestDir string) (string, []byte, error) { + pathSpec, err := util.ParsePathSpec(rawPathSpec) + if err != nil { + return "", nil, fmt.Errorf("error parsing path spec: %w", err) } - filepath, requestFile, err := util.OpenFileOrUrl(pathSpec, manifestDir) + requestFile, err := util.OpenFileOrUrl(pathSpec.Path, manifestDir) if err != nil { return "", nil, fmt.Errorf("error opening path: %s", err) } defer requestFile.Close() - requestTmpl, err := ioutil.ReadAll(requestFile) + requestTmpl, err := io.ReadAll(requestFile) if err != nil { return "", nil, fmt.Errorf("error loading file %s: %s", requestFile, err) } - return filepath, requestTmpl, nil + return pathSpec.Path, requestTmpl, nil } func LoadManifestDataAsObject(data any, manifestDir string, loader Loader) (filepath string, res any, err error) { diff --git a/pkg/lib/util/file.go b/pkg/lib/util/file.go index 6c8c75fb..b2f79840 100644 --- a/pkg/lib/util/file.go +++ b/pkg/lib/util/file.go @@ -16,22 +16,13 @@ var c = &http.Client{ } // OpenFileOrUrl opens either a local file or gives the resp.Body from a remote file -func OpenFileOrUrl(path, rootDir string) (string, io.ReadCloser, error) { - // Note that err is not propagated here since we're only checking - // *if* path is an @-Notation PathSpec. If so, we unwrap its path. - // If not, we assume we don't have an @-Notation at hand in the first place. - // Validation of PathSpec is performed further up in calling code. - pathSpec, err := ParsePathSpec(path) - if err == nil { - path = pathSpec.Path - } - +func OpenFileOrUrl(path, rootDir string) (io.ReadCloser, error) { if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") { io, err := openRemoteFile(path) - return path, io, err + return io, err } else { io, err := openLocalFile(path, rootDir) - return path, io, err + return io, err } } diff --git a/pkg/lib/util/file_test.go b/pkg/lib/util/file_test.go index 72763805..06db0536 100644 --- a/pkg/lib/util/file_test.go +++ b/pkg/lib/util/file_test.go @@ -3,7 +3,7 @@ package util import ( "crypto/md5" "fmt" - "io/ioutil" + "io" "net/http" "net/http/httptest" "path/filepath" @@ -57,13 +57,14 @@ func TestOpenFileOrUrl(t *testing.T) { for _, v := range tests { t.Run(fmt.Sprintf("%s", v.filename), func(t *testing.T) { - _, io, err := OpenFileOrUrl(v.filename, "") + file, err := OpenFileOrUrl(v.filename, "") if err != nil { if err.Error() != v.expError.Error() { t.Errorf("Got '%s' != '%s' Exp", err, v.expError) } } else { - data, err := ioutil.ReadAll(io) + defer file.Close() // FIXME: defer in for + data, err := io.ReadAll(file) if err != nil { t.Fatal(err) } @@ -88,7 +89,8 @@ func TestOpenLocalFile(t *testing.T) { if err != nil { t.Fatal("Root File: ", err) } - rootFile, err := ioutil.ReadAll(reader) + defer reader.Close() + rootFile, err := io.ReadAll(reader) if err != nil { t.Fatal("Root File: ", err) } @@ -100,7 +102,8 @@ func TestOpenLocalFile(t *testing.T) { if err != nil { t.Fatal("Manifest file: ", err) } - manifestFile, err := ioutil.ReadAll(reader) + defer reader.Close() + manifestFile, err := io.ReadAll(reader) if err != nil { t.Fatal("Manifest file: ", err) } @@ -113,7 +116,8 @@ func TestOpenLocalFile(t *testing.T) { if err != nil { t.Fatal("Binary file: ", err) } - binaryFile, err := ioutil.ReadAll(reader) + defer reader.Close() + binaryFile, err := io.ReadAll(reader) if err != nil { t.Fatal("Binary file: ", err) } diff --git a/test/proxy/read_from_proxies.json b/test/proxy/read_from_proxies.json index c0de5a93..bab82a6b 100644 --- a/test/proxy/read_from_proxies.json +++ b/test/proxy/read_from_proxies.json @@ -50,7 +50,7 @@ "type": "binary" }, "body": { - "md5sum": {{ md5sum "@../_res/assets/camera.jpg" | marshal }} + "md5sum": {{ md5sum "../_res/assets/camera.jpg" | marshal }} } } }, @@ -82,4 +82,4 @@ } } {{ end }} -] \ No newline at end of file +] From 5a0c57c706b19765ca4b11e4a30554dc663ef508 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Mon, 17 Jun 2024 17:10:16 +0200 Subject: [PATCH 28/32] BuildPolicy: Using io.Reader and io.Closer to ensure file handle can be closed --- pkg/lib/api/build_policies.go | 34 ++++++++++++++---------------- pkg/lib/api/build_policies_test.go | 7 +++--- pkg/lib/api/request.go | 19 +++++++++++++++-- pkg/lib/api/request_test.go | 9 ++++---- 4 files changed, 42 insertions(+), 27 deletions(-) diff --git a/pkg/lib/api/build_policies.go b/pkg/lib/api/build_policies.go index 7af90c86..1efc9621 100644 --- a/pkg/lib/api/build_policies.go +++ b/pkg/lib/api/build_policies.go @@ -13,7 +13,7 @@ import ( "github.com/programmfabrik/apitest/pkg/lib/util" ) -func buildMultipart(request Request) (additionalHeaders map[string]string, body io.Reader, err error) { +func buildMultipart(request Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) { additionalHeaders = make(map[string]string, 0) var buf = bytes.NewBuffer([]byte{}) @@ -24,7 +24,7 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body if ok { f, ok := val.(util.JsonString) if !ok { - return additionalHeaders, body, fmt.Errorf("file:filename should be a string") + return additionalHeaders, body, nil, fmt.Errorf("file:filename should be a string") } replaceFilename = &f } @@ -37,16 +37,16 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body rawPathSpec, ok := val.(util.JsonString) if !ok { - return additionalHeaders, body, fmt.Errorf("pathSpec should be a string") + return additionalHeaders, body, nil, fmt.Errorf("pathSpec should be a string") } pathSpec, err := util.ParsePathSpec(rawPathSpec) if err != nil { - return additionalHeaders, body, fmt.Errorf("pathSpec %s is not valid: %w", rawPathSpec, err) + return additionalHeaders, body, nil, fmt.Errorf("pathSpec %s is not valid: %w", rawPathSpec, err) } file, err := util.OpenFileOrUrl(pathSpec.Path, request.ManifestDir) if err != nil { - return additionalHeaders, nil, err + return additionalHeaders, nil, nil, err } defer file.Close() // FIXME: defer in for @@ -57,10 +57,10 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body part, err = w.CreateFormFile(key, *replaceFilename) } if err != nil { - return additionalHeaders, nil, err + return additionalHeaders, nil, nil, err } if _, err := io.Copy(part, file); err != nil { - return additionalHeaders, nil, err + return additionalHeaders, nil, nil, err } } err = w.Close() @@ -69,7 +69,7 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body return } -func buildUrlencoded(request Request) (additionalHeaders map[string]string, body io.Reader, err error) { +func buildUrlencoded(request Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) { additionalHeaders = make(map[string]string, 0) additionalHeaders["Content-Type"] = "application/x-www-form-urlencoded" formParams := url.Values{} @@ -84,11 +84,11 @@ func buildUrlencoded(request Request) (additionalHeaders map[string]string, body } } body = strings.NewReader(formParams.Encode()) - return additionalHeaders, body, nil + return additionalHeaders, body, nil, nil } -func buildRegular(request Request) (additionalHeaders map[string]string, body io.Reader, err error) { +func buildRegular(request Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) { additionalHeaders = make(map[string]string, 0) additionalHeaders["Content-Type"] = "application/json" @@ -97,18 +97,18 @@ func buildRegular(request Request) (additionalHeaders map[string]string, body io } else { bodyBytes, err := json.Marshal(request.Body) if err != nil { - return additionalHeaders, body, fmt.Errorf("error marshaling request body: %s", err) + return additionalHeaders, body, nil, fmt.Errorf("error marshaling request body: %s", err) } body = bytes.NewBuffer(bodyBytes) } - return additionalHeaders, body, nil + return additionalHeaders, body, nil, nil } -func buildFile(req Request) (map[string]string, io.Reader, error) { +func buildFile(req Request) (map[string]string, io.Reader, io.Closer, error) { headers := map[string]string{} if req.BodyFile == "" { - return nil, nil, errors.New(`Request.buildFile: Missing "body_file"`) + return nil, nil, nil, errors.New(`Request.buildFile: Missing "body_file"`) } path := req.BodyFile @@ -119,9 +119,7 @@ func buildFile(req Request) (map[string]string, io.Reader, error) { file, err := util.OpenFileOrUrl(path, req.ManifestDir) if err != nil { - return nil, nil, err + return nil, nil, nil, err } - // FIXME: file is left open AND passed out of function wihout ReadCloser! - - return headers, file, err + return headers, file, file, err } diff --git a/pkg/lib/api/build_policies_test.go b/pkg/lib/api/build_policies_test.go index be94ef7a..9a30f352 100644 --- a/pkg/lib/api/build_policies_test.go +++ b/pkg/lib/api/build_policies_test.go @@ -26,6 +26,7 @@ func TestBuildMultipart(t *testing.T) { ManifestDir: "test/", BodyType: "multipart", } + defer testRequest.Close() httpRequest, err := testRequest.buildHttpRequest() go_test_utils.ExpectNoError(t, err, "error building multipart request") @@ -49,7 +50,7 @@ func TestBuildMultipart_ErrPathSpec(t *testing.T) { ManifestDir: "test/path/", } - _, _, err := buildMultipart(testRequest) + _, _, _, err := buildMultipart(testRequest) if err == nil { t.Fatal("expected error") } @@ -66,7 +67,7 @@ func TestBuildMultipart_ErrPathSpecNoString(t *testing.T) { ManifestDir: "test/path/", } - _, _, err := buildMultipart(testRequest) + _, _, _, err := buildMultipart(testRequest) if err == nil { t.Fatal("expected error") } @@ -83,7 +84,7 @@ func TestBuildMultipart_FileDoesNotExist(t *testing.T) { ManifestDir: "test/path/", } - _, _, err := buildMultipart(testRequest) + _, _, _, err := buildMultipart(testRequest) if err == nil { t.Fatal("expected error") } diff --git a/pkg/lib/api/request.go b/pkg/lib/api/request.go index e3055f04..0c2f72fc 100755 --- a/pkg/lib/api/request.go +++ b/pkg/lib/api/request.go @@ -57,7 +57,8 @@ type Request struct { BodyFile string `yaml:"body_file" json:"body_file"` Body any `yaml:"body" json:"body"` - buildPolicy func(Request) (additionalHeaders map[string]string, body io.Reader, err error) + buildPolicy func(Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) + bodyCloser io.Closer ManifestDir string DataStore *datastore.Datastore } @@ -88,10 +89,16 @@ func (request Request) buildHttpRequest() (req *http.Request, err error) { return nil, errors.Wrapf(err, "Unable to buildHttpRequest with URL %q", requestUrl) } - additionalHeaders, body, err := request.buildPolicy(request) + if request.bodyCloser != nil { + // in case bodyCloser is already set, close the old body first + request.bodyCloser.Close() + } + + additionalHeaders, body, bodyCloser, err := request.buildPolicy(request) if err != nil { return req, fmt.Errorf("error executing buildpolicy: %s", err) } + request.bodyCloser = bodyCloser req, err = http.NewRequest(request.Method, requestUrl, body) if err != nil { @@ -266,6 +273,14 @@ func (request Request) buildHttpRequest() (req *http.Request, err error) { return req, nil } +func (request Request) Close() error { + if request.bodyCloser != nil { + return request.bodyCloser.Close() + } + + return nil +} + func (request Request) ToString(curl bool) (res string) { httpRequest, err := request.buildHttpRequest() if err != nil { diff --git a/pkg/lib/api/request_test.go b/pkg/lib/api/request_test.go index 8381647c..54c276b4 100755 --- a/pkg/lib/api/request_test.go +++ b/pkg/lib/api/request_test.go @@ -3,7 +3,6 @@ package api import ( "fmt" "io" - "io/ioutil" "net/http" "strings" "testing" @@ -22,17 +21,18 @@ func TestRequestBuildHttp(t *testing.T) { }, ServerURL: "serverUrl", } - request.buildPolicy = func(request Request) (ah map[string]string, b io.Reader, err error) { + defer request.Close() + request.buildPolicy = func(request Request) (ah map[string]string, b io.Reader, c io.Closer, err error) { ah = make(map[string]string) ah["mock-header"] = "application/mock" b = strings.NewReader("mock_body") - return ah, b, nil + return ah, b, c, nil } httpRequest, err := request.buildHttpRequest() go_test_utils.ExpectNoError(t, err, fmt.Sprintf("error building http-request: %s", err)) go_test_utils.AssertStringEquals(t, httpRequest.Header.Get("mock-header"), "application/mock") - assertBody, err := ioutil.ReadAll(httpRequest.Body) + assertBody, err := io.ReadAll(httpRequest.Body) go_test_utils.ExpectNoError(t, err, fmt.Sprintf("error reading http-request body: %s", err)) go_test_utils.AssertStringEquals(t, string(assertBody), "mock_body") @@ -88,6 +88,7 @@ func TestRequestBuildHttpWithCookie(t *testing.T) { Method: "GET", Cookies: reqCookies, } + defer request.Close() request.buildPolicy = buildRegular ds := datastore.NewStore(false) for key, val := range storeCookies { From 7ba5b47d49c7791b189fcf94759b2f9447003efb Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Mon, 17 Jun 2024 17:20:39 +0200 Subject: [PATCH 29/32] Fixing defer in for --- pkg/lib/api/build_policies.go | 27 +++++++++++++++++++-------- pkg/lib/util/file_test.go | 2 +- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/pkg/lib/api/build_policies.go b/pkg/lib/api/build_policies.go index 1efc9621..5097a309 100644 --- a/pkg/lib/api/build_policies.go +++ b/pkg/lib/api/build_policies.go @@ -30,25 +30,26 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body } additionalHeaders["Content-Type"] = w.FormDataContentType() - for key, val := range request.Body.(map[string]any) { + + createPart := func(key string, val any) error { if key == "file:filename" { - continue + return nil } rawPathSpec, ok := val.(util.JsonString) if !ok { - return additionalHeaders, body, nil, fmt.Errorf("pathSpec should be a string") + return fmt.Errorf("pathSpec should be a string") } pathSpec, err := util.ParsePathSpec(rawPathSpec) if err != nil { - return additionalHeaders, body, nil, fmt.Errorf("pathSpec %s is not valid: %w", rawPathSpec, err) + return fmt.Errorf("pathSpec %s is not valid: %w", rawPathSpec, err) } file, err := util.OpenFileOrUrl(pathSpec.Path, request.ManifestDir) if err != nil { - return additionalHeaders, nil, nil, err + return err } - defer file.Close() // FIXME: defer in for + defer file.Close() var part io.Writer if replaceFilename == nil { @@ -57,12 +58,22 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body part, err = w.CreateFormFile(key, *replaceFilename) } if err != nil { - return additionalHeaders, nil, nil, err + return err } if _, err := io.Copy(part, file); err != nil { - return additionalHeaders, nil, nil, err + return err } + + return nil } + + for key, val := range request.Body.(map[string]any) { + err = createPart(key, val) + if err != nil { + return additionalHeaders, body, bodyCloser, err + } + } + err = w.Close() body = bytes.NewBuffer(buf.Bytes()) diff --git a/pkg/lib/util/file_test.go b/pkg/lib/util/file_test.go index 06db0536..cca1ab14 100644 --- a/pkg/lib/util/file_test.go +++ b/pkg/lib/util/file_test.go @@ -63,7 +63,7 @@ func TestOpenFileOrUrl(t *testing.T) { t.Errorf("Got '%s' != '%s' Exp", err, v.expError) } } else { - defer file.Close() // FIXME: defer in for + defer file.Close() data, err := io.ReadAll(file) if err != nil { t.Fatal(err) From a01c29a0802740be1a38b4e6d09ff0ec86ce4b6b Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Mon, 17 Jun 2024 17:27:47 +0200 Subject: [PATCH 30/32] Moving loadFileFromPathSpec to PathSpec.LoadContents --- pkg/lib/template/util.go | 38 ++++++++++++-------------------------- pkg/lib/util/path_util.go | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/pkg/lib/template/util.go b/pkg/lib/template/util.go index 6ca75cf6..8eb79564 100644 --- a/pkg/lib/template/util.go +++ b/pkg/lib/template/util.go @@ -3,36 +3,18 @@ package template import ( "encoding/json" "fmt" - "io" "github.com/programmfabrik/apitest/pkg/lib/util" ) -func loadFileFromPathSpec(rawPathSpec, manifestDir string) (string, []byte, error) { - pathSpec, err := util.ParsePathSpec(rawPathSpec) - if err != nil { - return "", nil, fmt.Errorf("error parsing path spec: %w", err) - } - - requestFile, err := util.OpenFileOrUrl(pathSpec.Path, manifestDir) - if err != nil { - return "", nil, fmt.Errorf("error opening path: %s", err) - } - - defer requestFile.Close() - requestTmpl, err := io.ReadAll(requestFile) - - if err != nil { - return "", nil, fmt.Errorf("error loading file %s: %s", requestFile, err) - } - - return pathSpec.Path, requestTmpl, nil -} - func LoadManifestDataAsObject(data any, manifestDir string, loader Loader) (filepath string, res any, err error) { switch typedData := data.(type) { case string: - filepath, requestTmpl, err := loadFileFromPathSpec(typedData, manifestDir) + pathSpec, err := util.ParsePathSpec(typedData) + if err != nil { + return "", res, fmt.Errorf("error parsing pathSpec: %w", err) + } + requestTmpl, err := pathSpec.LoadContents(manifestDir) if err != nil { return "", res, fmt.Errorf("error loading fileFromPathSpec: %s", err) } @@ -53,7 +35,7 @@ func LoadManifestDataAsObject(data any, manifestDir string, loader Loader) (file } return "", res, fmt.Errorf("error unmarshalling: %s", err) } - return filepath, jsonObject, nil + return pathSpec.Path, jsonObject, nil case util.JsonObject: return "", typedData, nil case util.JsonArray: @@ -69,11 +51,15 @@ func LoadManifestDataAsRawJson(data any, manifestDir string) (filepath string, r err = res.UnmarshalJSON(typedData) return case string: - filepath, res, err := loadFileFromPathSpec(typedData, manifestDir) + pathSpec, err := util.ParsePathSpec(typedData) + if err != nil { + return "", res, fmt.Errorf("error parsing pathSpec: %w", err) + } + res, err := pathSpec.LoadContents(manifestDir) if err != nil { return "", res, fmt.Errorf("error loading fileFromPathSpec: %s", err) } - return filepath, res, nil + return pathSpec.Path, res, nil case util.JsonObject, util.JsonArray: jsonMar, err := json.Marshal(typedData) if err != nil { diff --git a/pkg/lib/util/path_util.go b/pkg/lib/util/path_util.go index a925e22f..f203aadb 100644 --- a/pkg/lib/util/path_util.go +++ b/pkg/lib/util/path_util.go @@ -2,6 +2,7 @@ package util import ( "fmt" + "io" "strconv" "strings" ) @@ -51,3 +52,19 @@ func IsPathSpec(s string) bool { _, err := ParsePathSpec(s) return err == nil } + +// Load loads the contents of the file pointed to by the PathSpec into a byte array. +func (ps PathSpec) LoadContents(manifestDir string) ([]byte, error) { + requestFile, err := OpenFileOrUrl(ps.Path, manifestDir) + if err != nil { + return nil, fmt.Errorf("error opening path: %w", err) + } + defer requestFile.Close() + + contents, err := io.ReadAll(requestFile) + if err != nil { + return nil, fmt.Errorf("error loading file at %q: %w", ps, err) + } + + return contents, nil +} From 6e37bb393a7479eb0a73a56f38c0d9e6cd6cc689 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Mon, 17 Jun 2024 17:33:55 +0200 Subject: [PATCH 31/32] Refactoring LoadManifest* functions to return PathSpec instead of string --- api_testsuite.go | 29 ++++++++++------------------- pkg/lib/template/util.go | 40 ++++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 39 deletions(-) diff --git a/api_testsuite.go b/api_testsuite.go index 28e6b933..5be3eb70 100644 --- a/api_testsuite.go +++ b/api_testsuite.go @@ -250,16 +250,18 @@ func (ats *Suite) parseAndRunTest( v any, testFilePath string, r *report.ReportElement, rootLoader template.Loader, allowParallelExec bool, ) bool { - // Parse PathSpec (if any) and determine number of parallel runs parallelRuns := 1 - if vStr, ok := v.(string); ok { - pathSpec, err := util.ParsePathSpec(vStr) - if err != nil { - logrus.Error(fmt.Errorf("test string is not a valid path spec: %w", err)) - return false - } - parallelRuns = pathSpec.ParallelRuns + // Get the Manifest with @ logic + referencedPathSpec, testRaw, err := template.LoadManifestDataAsRawJson(v, filepath.Dir(testFilePath)) + if err != nil { + r.SaveToReportLog(err.Error()) + logrus.Error(fmt.Errorf("can not LoadManifestDataAsRawJson (%s): %s", testFilePath, err)) + return false + } + if referencedPathSpec != nil { + testFilePath = filepath.Join(filepath.Dir(testFilePath), referencedPathSpec.Path) + parallelRuns = referencedPathSpec.ParallelRuns } // If parallel runs are requested, check that they're actually allowed @@ -268,17 +270,6 @@ func (ats *Suite) parseAndRunTest( return false } - // Get the Manifest with @ logic - referencedFilePath, testRaw, err := template.LoadManifestDataAsRawJson(v, filepath.Dir(testFilePath)) - if referencedFilePath != "" { - testFilePath = filepath.Join(filepath.Dir(testFilePath), referencedFilePath) - } - if err != nil { - r.SaveToReportLog(err.Error()) - logrus.Error(fmt.Errorf("can not LoadManifestDataAsRawJson (%s): %s", testFilePath, err)) - return false - } - // Execute test cases var successCount atomic.Uint32 var waitGroup sync.WaitGroup diff --git a/pkg/lib/template/util.go b/pkg/lib/template/util.go index 8eb79564..f899d476 100644 --- a/pkg/lib/template/util.go +++ b/pkg/lib/template/util.go @@ -7,22 +7,22 @@ import ( "github.com/programmfabrik/apitest/pkg/lib/util" ) -func LoadManifestDataAsObject(data any, manifestDir string, loader Loader) (filepath string, res any, err error) { +func LoadManifestDataAsObject(data any, manifestDir string, loader Loader) (pathSpec *util.PathSpec, res any, err error) { switch typedData := data.(type) { case string: - pathSpec, err := util.ParsePathSpec(typedData) + pathSpec, err = util.ParsePathSpec(typedData) if err != nil { - return "", res, fmt.Errorf("error parsing pathSpec: %w", err) + return nil, res, fmt.Errorf("error parsing pathSpec: %w", err) } requestTmpl, err := pathSpec.LoadContents(manifestDir) if err != nil { - return "", res, fmt.Errorf("error loading fileFromPathSpec: %s", err) + return nil, res, fmt.Errorf("error loading fileFromPathSpec: %s", err) } // We have json, and load it thereby into our apitest structure requestBytes, err := loader.Render(requestTmpl, manifestDir, nil) if err != nil { - return "", res, fmt.Errorf("error rendering request: %s", err) + return nil, res, fmt.Errorf("error rendering request: %s", err) } var jsonObject util.JsonObject @@ -31,45 +31,45 @@ func LoadManifestDataAsObject(data any, manifestDir string, loader Loader) (file if err = util.Unmarshal(requestBytes, &jsonObject); err != nil { if err = util.Unmarshal(requestBytes, &jsonArray); err == nil { - return filepath, jsonArray, nil + return pathSpec, jsonArray, nil } - return "", res, fmt.Errorf("error unmarshalling: %s", err) + return nil, res, fmt.Errorf("error unmarshalling: %s", err) } - return pathSpec.Path, jsonObject, nil + return pathSpec, jsonObject, nil case util.JsonObject: - return "", typedData, nil + return nil, typedData, nil case util.JsonArray: - return "", typedData, nil + return nil, typedData, nil default: - return "", res, fmt.Errorf("specification needs to be string[@...] or jsonObject but is: %s", data) + return nil, res, fmt.Errorf("specification needs to be string[@...] or jsonObject but is: %s", data) } } -func LoadManifestDataAsRawJson(data any, manifestDir string) (filepath string, res json.RawMessage, err error) { +func LoadManifestDataAsRawJson(data any, manifestDir string) (pathSpec *util.PathSpec, res json.RawMessage, err error) { switch typedData := data.(type) { case []byte: err = res.UnmarshalJSON(typedData) return case string: - pathSpec, err := util.ParsePathSpec(typedData) + pathSpec, err = util.ParsePathSpec(typedData) if err != nil { - return "", res, fmt.Errorf("error parsing pathSpec: %w", err) + return nil, res, fmt.Errorf("error parsing pathSpec: %w", err) } res, err := pathSpec.LoadContents(manifestDir) if err != nil { - return "", res, fmt.Errorf("error loading fileFromPathSpec: %s", err) + return nil, res, fmt.Errorf("error loading fileFromPathSpec: %s", err) } - return pathSpec.Path, res, nil + return pathSpec, res, nil case util.JsonObject, util.JsonArray: jsonMar, err := json.Marshal(typedData) if err != nil { - return "", res, fmt.Errorf("error marshaling: %s", err) + return nil, res, fmt.Errorf("error marshaling: %s", err) } if err = util.Unmarshal(jsonMar, &res); err != nil { - return "", res, fmt.Errorf("error unmarshalling: %s", err) + return nil, res, fmt.Errorf("error unmarshalling: %s", err) } - return "", res, nil + return nil, res, nil default: - return "", res, fmt.Errorf("specification needs to be string[@...] or jsonObject but is: %s", data) + return nil, res, fmt.Errorf("specification needs to be string[@...] or jsonObject but is: %s", data) } } From b6fb2e122e7efdf899f5b677d53595726a651f73 Mon Sep 17 00:00:00 2001 From: Lucas Hinderberger Date: Wed, 19 Jun 2024 11:09:32 +0200 Subject: [PATCH 32/32] Removing Closer from build_policies By instruction of Martin, I removed Closer from build_policies and added warning comments to the code instead. --- pkg/lib/api/build_policies.go | 26 ++++++++++++++------------ pkg/lib/api/build_policies_test.go | 7 +++---- pkg/lib/api/request.go | 23 ++++++----------------- pkg/lib/api/request_test.go | 6 ++---- 4 files changed, 25 insertions(+), 37 deletions(-) diff --git a/pkg/lib/api/build_policies.go b/pkg/lib/api/build_policies.go index 5097a309..271a6262 100644 --- a/pkg/lib/api/build_policies.go +++ b/pkg/lib/api/build_policies.go @@ -13,7 +13,7 @@ import ( "github.com/programmfabrik/apitest/pkg/lib/util" ) -func buildMultipart(request Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) { +func buildMultipart(request Request) (additionalHeaders map[string]string, body io.Reader, err error) { additionalHeaders = make(map[string]string, 0) var buf = bytes.NewBuffer([]byte{}) @@ -24,7 +24,7 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body if ok { f, ok := val.(util.JsonString) if !ok { - return additionalHeaders, body, nil, fmt.Errorf("file:filename should be a string") + return nil, nil, fmt.Errorf("file:filename should be a string") } replaceFilename = &f } @@ -70,7 +70,7 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body for key, val := range request.Body.(map[string]any) { err = createPart(key, val) if err != nil { - return additionalHeaders, body, bodyCloser, err + return nil, nil, err } } @@ -80,7 +80,7 @@ func buildMultipart(request Request) (additionalHeaders map[string]string, body return } -func buildUrlencoded(request Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) { +func buildUrlencoded(request Request) (additionalHeaders map[string]string, body io.Reader, err error) { additionalHeaders = make(map[string]string, 0) additionalHeaders["Content-Type"] = "application/x-www-form-urlencoded" formParams := url.Values{} @@ -95,11 +95,11 @@ func buildUrlencoded(request Request) (additionalHeaders map[string]string, body } } body = strings.NewReader(formParams.Encode()) - return additionalHeaders, body, nil, nil + return additionalHeaders, body, nil } -func buildRegular(request Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) { +func buildRegular(request Request) (additionalHeaders map[string]string, body io.Reader, err error) { additionalHeaders = make(map[string]string, 0) additionalHeaders["Content-Type"] = "application/json" @@ -108,18 +108,20 @@ func buildRegular(request Request) (additionalHeaders map[string]string, body io } else { bodyBytes, err := json.Marshal(request.Body) if err != nil { - return additionalHeaders, body, nil, fmt.Errorf("error marshaling request body: %s", err) + return nil, nil, fmt.Errorf("error marshaling request body: %s", err) } body = bytes.NewBuffer(bodyBytes) } - return additionalHeaders, body, nil, nil + return additionalHeaders, body, nil } -func buildFile(req Request) (map[string]string, io.Reader, io.Closer, error) { +// buildFile opens a file for use with buildPolicy. +// WARNING: This returns a file handle that must be closed! +func buildFile(req Request) (map[string]string, io.Reader, error) { headers := map[string]string{} if req.BodyFile == "" { - return nil, nil, nil, errors.New(`Request.buildFile: Missing "body_file"`) + return nil, nil, errors.New(`Request.buildFile: Missing "body_file"`) } path := req.BodyFile @@ -130,7 +132,7 @@ func buildFile(req Request) (map[string]string, io.Reader, io.Closer, error) { file, err := util.OpenFileOrUrl(path, req.ManifestDir) if err != nil { - return nil, nil, nil, err + return nil, nil, err } - return headers, file, file, err + return headers, file, err } diff --git a/pkg/lib/api/build_policies_test.go b/pkg/lib/api/build_policies_test.go index 9a30f352..be94ef7a 100644 --- a/pkg/lib/api/build_policies_test.go +++ b/pkg/lib/api/build_policies_test.go @@ -26,7 +26,6 @@ func TestBuildMultipart(t *testing.T) { ManifestDir: "test/", BodyType: "multipart", } - defer testRequest.Close() httpRequest, err := testRequest.buildHttpRequest() go_test_utils.ExpectNoError(t, err, "error building multipart request") @@ -50,7 +49,7 @@ func TestBuildMultipart_ErrPathSpec(t *testing.T) { ManifestDir: "test/path/", } - _, _, _, err := buildMultipart(testRequest) + _, _, err := buildMultipart(testRequest) if err == nil { t.Fatal("expected error") } @@ -67,7 +66,7 @@ func TestBuildMultipart_ErrPathSpecNoString(t *testing.T) { ManifestDir: "test/path/", } - _, _, _, err := buildMultipart(testRequest) + _, _, err := buildMultipart(testRequest) if err == nil { t.Fatal("expected error") } @@ -84,7 +83,7 @@ func TestBuildMultipart_FileDoesNotExist(t *testing.T) { ManifestDir: "test/path/", } - _, _, _, err := buildMultipart(testRequest) + _, _, err := buildMultipart(testRequest) if err == nil { t.Fatal("expected error") } diff --git a/pkg/lib/api/request.go b/pkg/lib/api/request.go index 0c2f72fc..5d3e1d8a 100755 --- a/pkg/lib/api/request.go +++ b/pkg/lib/api/request.go @@ -57,8 +57,7 @@ type Request struct { BodyFile string `yaml:"body_file" json:"body_file"` Body any `yaml:"body" json:"body"` - buildPolicy func(Request) (additionalHeaders map[string]string, body io.Reader, bodyCloser io.Closer, err error) - bodyCloser io.Closer + buildPolicy func(Request) (additionalHeaders map[string]string, body io.Reader, err error) ManifestDir string DataStore *datastore.Datastore } @@ -89,16 +88,14 @@ func (request Request) buildHttpRequest() (req *http.Request, err error) { return nil, errors.Wrapf(err, "Unable to buildHttpRequest with URL %q", requestUrl) } - if request.bodyCloser != nil { - // in case bodyCloser is already set, close the old body first - request.bodyCloser.Close() - } - - additionalHeaders, body, bodyCloser, err := request.buildPolicy(request) + // Note that buildPolicy may return a file handle that needs to be + // closed. According to standard library documentation, the NewRequest + // call below will take into account if body also happens to implement + // io.Closer. + additionalHeaders, body, err := request.buildPolicy(request) if err != nil { return req, fmt.Errorf("error executing buildpolicy: %s", err) } - request.bodyCloser = bodyCloser req, err = http.NewRequest(request.Method, requestUrl, body) if err != nil { @@ -273,14 +270,6 @@ func (request Request) buildHttpRequest() (req *http.Request, err error) { return req, nil } -func (request Request) Close() error { - if request.bodyCloser != nil { - return request.bodyCloser.Close() - } - - return nil -} - func (request Request) ToString(curl bool) (res string) { httpRequest, err := request.buildHttpRequest() if err != nil { diff --git a/pkg/lib/api/request_test.go b/pkg/lib/api/request_test.go index 54c276b4..d761ac04 100755 --- a/pkg/lib/api/request_test.go +++ b/pkg/lib/api/request_test.go @@ -21,12 +21,11 @@ func TestRequestBuildHttp(t *testing.T) { }, ServerURL: "serverUrl", } - defer request.Close() - request.buildPolicy = func(request Request) (ah map[string]string, b io.Reader, c io.Closer, err error) { + request.buildPolicy = func(request Request) (ah map[string]string, b io.Reader, err error) { ah = make(map[string]string) ah["mock-header"] = "application/mock" b = strings.NewReader("mock_body") - return ah, b, c, nil + return ah, b, nil } httpRequest, err := request.buildHttpRequest() go_test_utils.ExpectNoError(t, err, fmt.Sprintf("error building http-request: %s", err)) @@ -88,7 +87,6 @@ func TestRequestBuildHttpWithCookie(t *testing.T) { Method: "GET", Cookies: reqCookies, } - defer request.Close() request.buildPolicy = buildRegular ds := datastore.NewStore(false) for key, val := range storeCookies {