From 0feeb945fc4d8a17685d106b02a7315805c6761b Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 15 Feb 2018 19:15:08 -0600 Subject: [PATCH 1/3] Fix some tests for Windows This commit incorporates some tests into the main TestWebhook framework. New features to TestWebhook: - Check log output against Regexp - Add Testing sub-tests Updates #225 --- test/hooks.json.tmpl | 24 +++++ test/hooks.yaml.tmpl | 29 ++++-- webhook_test.go | 216 ++++++++++++++++++++----------------------- 3 files changed, 149 insertions(+), 120 deletions(-) diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index d65d1d3..2fd90da 100644 --- a/test/hooks.json.tmpl +++ b/test/hooks.json.tmpl @@ -180,5 +180,29 @@ "execute-command": "{{ .Hookecho }}", "include-command-output-in-response": true, "include-command-output-in-response-on-error": true + }, + { + "id": "static-params-ok", + "execute-command": "{{ .Hookecho }}", + "response-message": "success", + "include-command-output-in-response": true, + "pass-arguments-to-command": [ + { + "source": "string", + "name": "passed" + } + ], + }, + { + "id": "warn-on-space", + "execute-command": "{{ .Hookecho }} foo", + "response-message": "success", + "include-command-output-in-response": true, + "pass-arguments-to-command": [ + { + "source": "string", + "name": "passed" + } + ], } ] diff --git a/test/hooks.yaml.tmpl b/test/hooks.yaml.tmpl index c0377e5..c2fffcd 100644 --- a/test/hooks.yaml.tmpl +++ b/test/hooks.yaml.tmpl @@ -1,4 +1,5 @@ -- trigger-rule: +- id: github + trigger-rule: and: - match: parameter: @@ -23,9 +24,10 @@ pass-environment-to-command: - source: payload name: head_commit.timestamp - id: github command-working-directory: / -- trigger-rule: + +- id: bitbucket + trigger-rule: and: - match: parameter: @@ -52,9 +54,10 @@ execute-command: '{{ .Hookecho }}' response-message: success include-command-output-in-response: false - id: bitbucket command-working-directory: / -- trigger-rule: + +- id: gitlab + trigger-rule: match: parameter: source: payload @@ -71,25 +74,28 @@ execute-command: '{{ .Hookecho }}' response-message: success include-command-output-in-response: true - id: gitlab command-working-directory: / + - id: capture-command-output-on-success-not-by-default pass-arguments-to-command: - source: string name: exit=0 execute-command: '{{ .Hookecho }}' + - id: capture-command-output-on-success-yes-with-flag pass-arguments-to-command: - source: string name: exit=0 execute-command: '{{ .Hookecho }}' include-command-output-in-response: true + - id: capture-command-output-on-error-not-by-default pass-arguments-to-command: - source: string name: exit=1 execute-command: '{{ .Hookecho }}' include-command-output-in-response: true + - id: capture-command-output-on-error-yes-with-extra-flag pass-arguments-to-command: - source: string @@ -97,3 +103,14 @@ execute-command: '{{ .Hookecho }}' include-command-output-in-response: true include-command-output-in-response-on-error: true + +- id: static-params-ok + execute-command: '{{ .Hookecho }}' + include-command-output-in-response: true + pass-arguments-to-command: + - source: string + name: passed + +- id: warn-on-space + execute-command: '{{ .Hookecho }} foo' + include-command-output-in-response: true \ No newline at end of file diff --git a/webhook_test.go b/webhook_test.go index 4373b16..4e04ab3 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -21,15 +21,26 @@ import ( ) func TestStaticParams(t *testing.T) { + // FIXME(moorereason): incorporate this test into TestWebhook. + // Need to be able to execute a binary with a space in the filename. + if runtime.GOOS == "windows" { + t.Skip("Skipping on Windows") + } spHeaders := make(map[string]interface{}) spHeaders["User-Agent"] = "curl/7.54.0" spHeaders["Accept"] = "*/*" - // case 1: correct entry + // case 2: binary with spaces in its name + err := os.Symlink("/bin/echo", "/tmp/with space") + if err != nil { + t.Fatalf("%v", err) + } + defer os.Remove("/tmp/with space") + spHook := &hook.Hook{ - ID: "static-params-ok", - ExecuteCommand: "/bin/echo", + ID: "static-params-name-space", + ExecuteCommand: "/tmp/with space", CommandWorkingDirectory: "/tmp", ResponseMessage: "success", CaptureCommandOutput: true, @@ -41,68 +52,14 @@ func TestStaticParams(t *testing.T) { b := &bytes.Buffer{} log.SetOutput(b) - s, err := handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) + _, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) if err != nil { t.Fatalf("Unexpected error: %v\n", err) } - matched, _ := regexp.MatchString("(?s).*command output: passed.*static-params-ok", b.String()) - if !matched { - t.Fatalf("Unexpected log output:\n%s", b) - } - - // case 2: binary with spaces in its name - err = os.Symlink("/bin/echo", "/tmp/with space") - if err != nil { - t.Fatalf("%v", err) - } - defer os.Remove("/tmp/with space") - - spHook = &hook.Hook{ - ID: "static-params-name-space", - ExecuteCommand: "/tmp/with space", - CommandWorkingDirectory: "/tmp", - ResponseMessage: "success", - CaptureCommandOutput: true, - PassArgumentsToCommand: []hook.Argument{ - hook.Argument{Source: "string", Name: "passed"}, - }, - } - - b = &bytes.Buffer{} - log.SetOutput(b) - - s, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) - if err != nil { - t.Fatalf("Unexpected error: %v\n", err) - } - matched, _ = regexp.MatchString("(?s)command output: .*static-params-name-space", b.String()) + matched, _ := regexp.MatchString("(?s)command output: .*static-params-name-space", b.String()) if !matched { t.Fatalf("Unexpected log output:\n%sn", b) } - - // case 3: parameters specified in execute-command - spHook = &hook.Hook{ - ID: "static-params-bad", - ExecuteCommand: "/bin/echo success", - CommandWorkingDirectory: "/tmp", - ResponseMessage: "success", - CaptureCommandOutput: true, - PassArgumentsToCommand: []hook.Argument{ - hook.Argument{Source: "string", Name: "failed"}, - }, - } - - b = &bytes.Buffer{} - log.SetOutput(b) - - s, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) - if err == nil { - t.Fatalf("Error expected, but none returned: %s\n", s) - } - matched, _ = regexp.MatchString("(?s)unable to locate command: ..bin.echo success.*use 'pass-arguments-to-command'", b.String()) - if !matched { - t.Fatalf("Unexpected log output:\n%s\n", b) - } } func TestWebhook(t *testing.T) { @@ -110,17 +67,20 @@ func TestWebhook(t *testing.T) { defer cleanupHookecho() for _, hookTmpl := range []string{"test/hooks.json.tmpl", "test/hooks.yaml.tmpl"} { - config, cleanupConfig := genConfig(t, hookecho, hookTmpl) - defer cleanupConfig() + configPath, cleanupConfigFn := genConfig(t, hookecho, hookTmpl) + defer cleanupConfigFn() - webhook, cleanupWebhook := buildWebhook(t) - defer cleanupWebhook() + webhook, cleanupWebhookFn := buildWebhook(t) + defer cleanupWebhookFn() ip, port := serverAddress(t) - args := []string{fmt.Sprintf("-hooks=%s", config), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} + args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} + + // Setup a buffer for capturing webhook logs for later evaluation + b := &bytes.Buffer{} cmd := exec.Command(webhook, args...) - //cmd.Stderr = os.Stderr // uncomment to see verbose output + cmd.Stderr = b cmd.Env = webhookEnv() cmd.Args[0] = "webhook" if err := cmd.Start(); err != nil { @@ -131,69 +91,83 @@ func TestWebhook(t *testing.T) { waitForServerReady(t, ip, port) for _, tt := range hookHandlerTests { - url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id) + t.Run(tt.desc, func(t *testing.T) { - req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body))) - if err != nil { - t.Errorf("New request failed: %s", err) - } + url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id) - for k, v := range tt.headers { - req.Header.Add(k, v) - } + req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body))) + if err != nil { + t.Errorf("New request failed: %s", err) + } - var res *http.Response + for k, v := range tt.headers { + req.Header.Add(k, v) + } - if tt.urlencoded == true { - req.Header.Add("Content-Type", "application/x-www-form-urlencoded") - } else { - req.Header.Add("Content-Type", "application/json") - } + var res *http.Response - client := &http.Client{} - res, err = client.Do(req) - if err != nil { - t.Errorf("client.Do failed: %s", err) - } + if tt.urlencoded == true { + req.Header.Add("Content-Type", "application/x-www-form-urlencoded") + } else { + req.Header.Add("Content-Type", "application/json") + } - body, err := ioutil.ReadAll(res.Body) - res.Body.Close() - if err != nil { - t.Errorf("POST %q: failed to ready body: %s", tt.desc, err) - } + client := &http.Client{} + res, err = client.Do(req) + if err != nil { + t.Errorf("client.Do failed: %s", err) + } - if res.StatusCode != tt.respStatus || string(body) != tt.respBody { - t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body) - } + body, err := ioutil.ReadAll(res.Body) + res.Body.Close() + if err != nil { + t.Errorf("POST %q: failed to ready body: %s", tt.desc, err) + } + + if res.StatusCode != tt.respStatus || string(body) != tt.respBody { + t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body) + } + + if tt.logMatch == "" { + b.Reset() + return + } + + matched, _ := regexp.MatchString(tt.logMatch, b.String()) + if !matched { + t.Fatalf("Unexpected log output:\n%s", b) + } + b.Reset() + }) } } } -func buildHookecho(t *testing.T) (bin string, cleanup func()) { +func buildHookecho(t *testing.T) (binPath string, cleanupFn func()) { tmp, err := ioutil.TempDir("", "hookecho-test-") if err != nil { t.Fatal(err) } defer func() { - if cleanup == nil { + if cleanupFn == nil { os.RemoveAll(tmp) } }() - bin = filepath.Join(tmp, "hookecho") + binPath = filepath.Join(tmp, "hookecho") if runtime.GOOS == "windows" { - bin += ".exe" + binPath += ".exe" } - cmd := exec.Command("go", "build", "-o", bin, "test/hookecho.go") + cmd := exec.Command("go", "build", "-o", binPath, "test/hookecho.go") if err := cmd.Run(); err != nil { t.Fatalf("Building hookecho: %v", err) } - return bin, func() { os.RemoveAll(tmp) } + return binPath, func() { os.RemoveAll(tmp) } } -func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cleanup func()) { +func genConfig(t *testing.T, bin string, hookTemplate string) (configPath string, cleanupFn func()) { tmpl := template.Must(template.ParseFiles(hookTemplate)) tmp, err := ioutil.TempDir("", "webhook-config-") @@ -201,7 +175,7 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl t.Fatal(err) } defer func() { - if cleanup == nil { + if cleanupFn == nil { os.RemoveAll(tmp) } }() @@ -215,7 +189,11 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl } defer file.Close() - data := struct{ Hookecho string }{filepath.ToSlash(bin)} + data := struct{ Hookecho string }{filepath.FromSlash(bin)} + if runtime.GOOS == "windows" { + // Simulate escaped backslashes on Windows. + data.Hookecho = strings.Replace(data.Hookecho, `\`, `\\`, -1) + } if err := tmpl.Execute(file, data); err != nil { t.Fatalf("Executing template: %v", err) } @@ -223,28 +201,28 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl return path, func() { os.RemoveAll(tmp) } } -func buildWebhook(t *testing.T) (bin string, cleanup func()) { +func buildWebhook(t *testing.T) (binPath string, cleanupFn func()) { tmp, err := ioutil.TempDir("", "webhook-test-") if err != nil { t.Fatal(err) } defer func() { - if cleanup == nil { + if cleanupFn == nil { os.RemoveAll(tmp) } }() - bin = filepath.Join(tmp, "webhook") + binPath = filepath.Join(tmp, "webhook") if runtime.GOOS == "windows" { - bin += ".exe" + binPath += ".exe" } - cmd := exec.Command("go", "build", "-o", bin) + cmd := exec.Command("go", "build", "-o", binPath) if err := cmd.Run(); err != nil { t.Fatalf("Building webhook: %v", err) } - return bin, func() { os.RemoveAll(tmp) } + return binPath, func() { os.RemoveAll(tmp) } } func serverAddress(t *testing.T) (string, string) { @@ -313,6 +291,7 @@ var hookHandlerTests = []struct { respStatus int respBody string + logMatch string }{ { "github", @@ -467,6 +446,7 @@ var hookHandlerTests = []struct { `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 `, + ``, }, { "bitbucket", // bitbucket sends their payload using uriencoded params. @@ -476,6 +456,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 true, http.StatusOK, `success`, + ``, }, { "gitlab", @@ -527,6 +508,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 http.StatusOK, `arg: b6568db1bc1dcd7f8b4d5a946b0b91f9dacd7327 John Smith john@example.com `, + ``, }, { @@ -567,6 +549,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 `, + ``, }, { @@ -605,20 +588,25 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 http.StatusOK, `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz `, + ``, }, // test with custom return code - {"empty payload", "github", nil, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`}, + {"empty payload", "github", nil, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, // test with custom invalid http code, should default to 200 OK - {"empty payload", "bitbucket", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`}, + {"empty payload", "bitbucket", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``}, // test with no configured http return code, should default to 200 OK - {"empty payload", "gitlab", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`}, + {"empty payload", "gitlab", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``}, // test capturing command output - {"don't capture output on success by default", "capture-command-output-on-success-not-by-default", nil, `{}`, false, http.StatusOK, ``}, + {"don't capture output on success by default", "capture-command-output-on-success-not-by-default", nil, `{}`, false, http.StatusOK, ``, ``}, {"capture output on success with flag set", "capture-command-output-on-success-yes-with-flag", nil, `{}`, false, http.StatusOK, `arg: exit=0 -`}, - {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, `{}`, false, http.StatusInternalServerError, `Error occurred while executing the hook's command. Please check your logs for more details.`}, +`, ``}, + {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, `{}`, false, http.StatusInternalServerError, `Error occurred while executing the hook's command. Please check your logs for more details.`, ``}, {"capture output on error with extra flag set", "capture-command-output-on-error-yes-with-extra-flag", nil, `{}`, false, http.StatusInternalServerError, `arg: exit=1 -`}, +`, ``}, + + // Check logs + {"static params should pass", "static-params-ok", nil, `{}`, false, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`}, + {"command with space logs warning", "warn-on-space", nil, `{}`, false, http.StatusInternalServerError, "Error occurred while executing the hook's command. Please check your logs for more details.", `(?s)unable to locate command.*use 'pass[-]arguments[-]to[-]command' to specify args`}, } From 337621998e791cdd6cc90ca96780e1a102c42ed8 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 15 Feb 2018 19:53:28 -0600 Subject: [PATCH 2/3] Fix race in TestWebhook Previous commit misused a bytes.Buffer. Protect the buffer with a mutex. --- webhook_test.go | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/webhook_test.go b/webhook_test.go index 4e04ab3..0c3016a 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -13,6 +13,7 @@ import ( "regexp" "runtime" "strings" + "sync" "testing" "text/template" "time" @@ -66,18 +67,18 @@ func TestWebhook(t *testing.T) { hookecho, cleanupHookecho := buildHookecho(t) defer cleanupHookecho() + webhook, cleanupWebhookFn := buildWebhook(t) + defer cleanupWebhookFn() + for _, hookTmpl := range []string{"test/hooks.json.tmpl", "test/hooks.yaml.tmpl"} { configPath, cleanupConfigFn := genConfig(t, hookecho, hookTmpl) defer cleanupConfigFn() - webhook, cleanupWebhookFn := buildWebhook(t) - defer cleanupWebhookFn() - ip, port := serverAddress(t) args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} // Setup a buffer for capturing webhook logs for later evaluation - b := &bytes.Buffer{} + b := &buffer{} cmd := exec.Command(webhook, args...) cmd.Stderr = b @@ -610,3 +611,33 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 {"static params should pass", "static-params-ok", nil, `{}`, false, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`}, {"command with space logs warning", "warn-on-space", nil, `{}`, false, http.StatusInternalServerError, "Error occurred while executing the hook's command. Please check your logs for more details.", `(?s)unable to locate command.*use 'pass[-]arguments[-]to[-]command' to specify args`}, } + +// buffer provides a concurrency-safe bytes.Buffer to tests above. +type buffer struct { + b bytes.Buffer + m sync.Mutex +} + +func (b *buffer) Read(p []byte) (n int, err error) { + b.m.Lock() + defer b.m.Unlock() + return b.b.Read(p) +} + +func (b *buffer) Write(p []byte) (n int, err error) { + b.m.Lock() + defer b.m.Unlock() + return b.b.Write(p) +} + +func (b *buffer) String() string { + b.m.Lock() + defer b.m.Unlock() + return b.b.String() +} + +func (b *buffer) Reset() { + b.m.Lock() + defer b.m.Unlock() + b.b.Reset() +} From 471c849c503f35527fa4b7cfb6a64c979a1888ee Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 14:36:08 -0600 Subject: [PATCH 3/3] Fix another race condition in TestWebhook There's the potential for a race condition where we try to read the logs buffer before the logs have been flushed by the webhook process. Kill the process to flush the logs before testing against the log buffer. --- webhook_test.go | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/webhook_test.go b/webhook_test.go index 0c3016a..6113b9b 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -74,26 +74,26 @@ func TestWebhook(t *testing.T) { configPath, cleanupConfigFn := genConfig(t, hookecho, hookTmpl) defer cleanupConfigFn() - ip, port := serverAddress(t) - args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} - - // Setup a buffer for capturing webhook logs for later evaluation - b := &buffer{} - - cmd := exec.Command(webhook, args...) - cmd.Stderr = b - cmd.Env = webhookEnv() - cmd.Args[0] = "webhook" - if err := cmd.Start(); err != nil { - t.Fatalf("failed to start webhook: %s", err) - } - defer killAndWait(cmd) - - waitForServerReady(t, ip, port) - for _, tt := range hookHandlerTests { t.Run(tt.desc, func(t *testing.T) { + ip, port := serverAddress(t) + args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} + + // Setup a buffer for capturing webhook logs for later evaluation + b := &buffer{} + + cmd := exec.Command(webhook, args...) + cmd.Stderr = b + cmd.Env = webhookEnv() + cmd.Args[0] = "webhook" + if err := cmd.Start(); err != nil { + t.Fatalf("failed to start webhook: %s", err) + } + defer killAndWait(cmd) + + waitForServerReady(t, ip, port) + url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id) req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body))) @@ -130,15 +130,19 @@ func TestWebhook(t *testing.T) { } if tt.logMatch == "" { - b.Reset() return } + // There's the potential for a race condition below where we + // try to read the logs buffer b before the logs have been + // flushed by the webhook process. Kill the process to flush + // the logs. + killAndWait(cmd) + matched, _ := regexp.MatchString(tt.logMatch, b.String()) if !matched { - t.Fatalf("Unexpected log output:\n%s", b) + t.Errorf("failed log match for %q (id: %s):\nmatch pattern: %q\ngot:\n%s", tt.desc, tt.id, tt.logMatch, b) } - b.Reset() }) } } @@ -267,6 +271,10 @@ func waitForServer(t *testing.T, url string, status int, timeout time.Duration) } func killAndWait(cmd *exec.Cmd) { + if cmd == nil || cmd.ProcessState != nil && cmd.ProcessState.Exited() { + return + } + cmd.Process.Kill() cmd.Wait() }