Skip to content

Commit c6ee0c2

Browse files
ZNeumannZNeumann
authored
test(integration): improve valgrind (#1065)
This PR: 1) adds the option to run valgrind on integration tests in GHA 2) adds the ability to run valgrind on CGI tests 3) no longer runs valgrind on SKIPIFs 4) changes the valgrind lock mechanism to allow subprocesses (i.e. curl) to not be locked out
1 parent f2c18ed commit c6ee0c2

File tree

5 files changed

+172
-29
lines changed

5 files changed

+172
-29
lines changed

‎.github/workflows/make-integration-tests.yml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ on:
2424
description: 'runner type'
2525
required: true
2626
type: string
27+
use-valgrind:
28+
required: true
29+
default: false
30+
type: boolean
2731

2832
jobs:
2933
test-agent:
@@ -77,7 +81,7 @@ jobs:
7781
run: |
7882
docker exec \
7983
-e PHPS=${{matrix.php}} \
80-
-e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so" \
84+
-e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so ${{ inputs.use-valgrind && '-valgrind /usr/bin/valgrind' }}" \
8185
-e APP_supportability=${{secrets.APP_SUPPORTABILITY}} \
8286
-e ACCOUNT_supportability=${{secrets.ACCOUNT_SUPPORTABILITY}} \
8387
-e ACCOUNT_supportability_trusted=${{secrets.ACCOUNT_SUPPORTABILITY_TRUSTED}} \

‎.github/workflows/run-integration-tests.yml

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ on:
2626
options:
2727
- ubuntu-22.04-arm
2828
- ubuntu-24.04-arm
29-
29+
use-valgrind:
30+
description: 'Use valgrind?'
31+
required: true
32+
default: false
33+
type: boolean
3034
jobs:
3135
build-test-runner-amd64:
3236
uses: ./.github/workflows/make-for-platform-on-arch.yml
@@ -78,6 +82,7 @@ jobs:
7882
with:
7983
origin: ${{ inputs.origin }}
8084
ref: ${{ inputs.ref }}
85+
use-valgrind: ${{ inputs.use-valgrind }}
8186
arch: amd64
8287
runs-on: ubuntu-latest
8388
continue-on-error: false
@@ -88,6 +93,7 @@ jobs:
8893
with:
8994
origin: ${{ inputs.origin }}
9095
ref: ${{ inputs.ref }}
96+
use-valgrind: ${{ inputs.use-valgrind }}
9197
arch: arm64
9298
runs-on: ${{inputs.runner-type}}
9399
continue-on-error: true

‎daemon/cmd/integration_runner/main.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,10 @@ func main() {
344344

345345
ctx = integration.NewContext(*flagPHP, *flagCGI)
346346
ctx.Valgrind = *flagValgrind
347+
if ctx.Valgrind != "" && *flagWorkers != 1 {
348+
fmt.Fprintf(os.Stderr, "cannot run valgrind on multiple workers\n")
349+
os.Exit(1)
350+
}
347351
ctx.Timeout = *flagTimeout
348352

349353
// Settings common to all tests.

‎daemon/internal/newrelic/integration/transaction.go

Lines changed: 61 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error
4848

4949
args := phpArgs(nil, filepath.Base(src.Name()), false, settings)
5050

51-
if ctx.Valgrind != "" {
51+
if ctx.Valgrind != "" && settings["newrelic.appname"] != "skipif" {
5252
txn = &ValgrindCLI{
5353
CLI: CLI{
5454
Path: ctx.PHP,
@@ -81,6 +81,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error
8181
// See: https://tools.ietf.org/html/rfc3875
8282
func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx *Context) (Tx, error) {
8383
var err error
84+
var txn Tx
8485

8586
req := &http.Request{
8687
Method: env["REQUEST_METHOD"],
@@ -105,34 +106,69 @@ func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx
105106
return nil, fmt.Errorf("unable to create cgi request: %v", err)
106107
}
107108

108-
tx := &CGI{
109-
request: req,
110-
handler: &cgi.Handler{
111-
Path: ctx.CGI,
112-
Dir: src.Dir(),
113-
Args: phpArgs(nil, "", false, settings),
114-
},
115-
}
109+
if ctx.Valgrind != "" {
110+
tx := &ValgrindCGI{
111+
CGI: CGI{
112+
request: req,
113+
handler: &cgi.Handler{
114+
Path: ctx.CGI,
115+
Dir: src.Dir(),
116+
Args: phpArgs(nil, "", false, settings),
117+
},
118+
},
119+
Valgrind: ctx.Valgrind,
120+
Timeout: ctx.Timeout,
121+
}
122+
tx.handler.Env = append(tx.handler.Env,
123+
"SCRIPT_FILENAME="+scriptFile,
124+
"SCRIPT_NAME=/"+filepath.Base(src.Name()),
125+
"REDIRECT_STATUS=200")
126+
tx.handler.Env = append(tx.handler.Env, flatten(env)...)
127+
128+
// If the environment includes a REQUEST_URI and doesn't include an explicit
129+
// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
130+
// attempting to guess what the PATH_INFO should be.
131+
if ruri, ok := env["REQUEST_URI"]; ok {
132+
if _, ok := env["PATH_INFO"]; !ok {
133+
tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
134+
}
135+
}
116136

117-
tx.handler.Env = append(tx.handler.Env,
118-
"SCRIPT_FILENAME="+scriptFile,
119-
"SCRIPT_NAME=/"+filepath.Base(src.Name()),
120-
"REDIRECT_STATUS=200")
121-
tx.handler.Env = append(tx.handler.Env, flatten(env)...)
122-
123-
// If the environment includes a REQUEST_URI and doesn't include an explicit
124-
// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
125-
// attempting to guess what the PATH_INFO should be.
126-
if ruri, ok := env["REQUEST_URI"]; ok {
127-
if _, ok := env["PATH_INFO"]; !ok {
128-
tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
137+
if !src.IsFile() {
138+
return withTempFile(tx, src.Name(), src.Bytes()), nil
139+
}
140+
txn = tx
141+
} else {
142+
tx := &CGI{
143+
request: req,
144+
handler: &cgi.Handler{
145+
Path: ctx.CGI,
146+
Dir: src.Dir(),
147+
Args: phpArgs(nil, "", false, settings),
148+
},
149+
}
150+
tx.handler.Env = append(tx.handler.Env,
151+
"SCRIPT_FILENAME="+scriptFile,
152+
"SCRIPT_NAME=/"+filepath.Base(src.Name()),
153+
"REDIRECT_STATUS=200")
154+
tx.handler.Env = append(tx.handler.Env, flatten(env)...)
155+
156+
// If the environment includes a REQUEST_URI and doesn't include an explicit
157+
// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
158+
// attempting to guess what the PATH_INFO should be.
159+
if ruri, ok := env["REQUEST_URI"]; ok {
160+
if _, ok := env["PATH_INFO"]; !ok {
161+
tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
162+
}
129163
}
130-
}
131164

132-
if !src.IsFile() {
133-
return withTempFile(tx, src.Name(), src.Bytes()), nil
165+
if !src.IsFile() {
166+
return withTempFile(tx, src.Name(), src.Bytes()), nil
167+
}
168+
txn = tx
134169
}
135-
return tx, nil
170+
171+
return txn, nil
136172
}
137173

138174
// phpArgs builds the command line for a PHP process.

‎daemon/internal/newrelic/integration/valgrind.go

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111
"io/ioutil"
1212
"net/http"
13+
"net/http/httptest"
1314
"sync"
1415
"time"
1516

@@ -24,6 +25,12 @@ type ValgrindCLI struct {
2425
Timeout time.Duration
2526
}
2627

28+
type ValgrindCGI struct {
29+
CGI
30+
Valgrind string
31+
Timeout time.Duration
32+
}
33+
2734
func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
2835
if len(tx.Path) == 0 {
2936
return nil, []byte("skip: executable not specified"), nil
@@ -34,8 +41,13 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
3441
// valgrind reports to their tests in the same way we use the appname
3542
// to link the transaction data. Failing that, perhaps we could use
3643
// the valgrind process's pid instead.
37-
valgrindMu.Lock()
38-
defer valgrindMu.Unlock()
44+
//
45+
// We prevent concurrent invocations by disallowing valgrind when
46+
// the threads argument is greater than 1. Previously, we used the
47+
// commented out locks below. This was changed to allow subprocesses
48+
// (i.e. curl) to run without deadlocking.
49+
//valgrindMu.Lock()
50+
//defer valgrindMu.Unlock()
3951

4052
cmd := valgrind.Memcheck(tx.Valgrind, "--quiet")
4153
cmd.Args = append(cmd.Args, "--xml=yes")
@@ -105,6 +117,87 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
105117
return nil, output, err
106118
}
107119

120+
func (tx *ValgrindCGI) Execute() (http.Header, []byte, error) {
121+
if len(tx.handler.Path) == 0 {
122+
return nil, []byte("skip: executable not specified"), nil
123+
}
124+
125+
// For now, we don't have a mechanism to handle concurrent invocations
126+
// of Valgrind. In the future, we could use the appname to connect
127+
// valgrind reports to their tests in the same way we use the appname
128+
// to link the transaction data. Failing that, perhaps we could use
129+
// the valgrind process's pid instead.
130+
//
131+
// We prevent concurrent invocations by disallowing valgrind when
132+
// the threads argument is greater than 1. Previously, we used the
133+
// commented out locks below. This was changed to allow subprocesses
134+
// (i.e. curl) to run without deadlocking.
135+
//valgrindMu.Lock()
136+
//defer valgrindMu.Unlock()
137+
138+
cmd := valgrind.Memcheck(tx.Valgrind, "--quiet")
139+
cmd.Args = append(cmd.Args, "--xml=yes")
140+
cmd.Args = append(cmd.Args, "--xml-socket="+valgrindLn.Addr().String())
141+
cmd.Args = append(cmd.Args, "--")
142+
cmd.Args = append(cmd.Args, tx.handler.Path)
143+
if len(tx.handler.Args) > 0 {
144+
cmd.Args = append(cmd.Args, tx.handler.Args...)
145+
}
146+
147+
log.Debugf("command: %v", cmd)
148+
149+
// Replace the handler with valgrind
150+
tx.handler.Path = cmd.Path
151+
// The first Arg is the Path.
152+
// ServeHTTP will re-add Path
153+
// to the front of Args
154+
tx.handler.Args = cmd.Args[1:]
155+
156+
ch := make(chan resultOrError, 1)
157+
go func() {
158+
var result resultOrError
159+
result.R, result.E = acceptOneReport(tx.Timeout)
160+
ch <- result
161+
}()
162+
163+
resp := httptest.NewRecorder()
164+
tx.handler.ServeHTTP(resp, tx.request)
165+
166+
vgOutput := <-ch
167+
168+
// Append the output from Valgrind to the test output.
169+
//
170+
// TODO: Eventually we want to report valgrind output separately, so we can
171+
// treat valgrind errors similarly to failed test expectations. i.e. We'd
172+
// like to add them to Test.Failures. After all, each test has an implicit
173+
// expectation that it will not exhibit memory bugs!
174+
//
175+
// TODO: Once the former is in place, we should be able to parse the memory
176+
// leak reports produced by the Zend Memory Manager from the test output and
177+
// report them the same way as valgrind errors.
178+
output := resp.Body.Bytes()
179+
if vgOutput.R != nil && len(vgOutput.R.Errors) > 0 {
180+
// Safe to ignore the error here, Report.MarshalText() never fails.
181+
data, _ := vgOutput.R.MarshalText()
182+
output = append(output, '\n')
183+
output = append(output, data...)
184+
}
185+
186+
// Ensure a non-nil error is returned when valgrind detects errors.
187+
// Otherwise, the test could be marked as passing if it does not have
188+
// any expectations on the test output. This sucks.
189+
//
190+
// TODO: Remove this when valgrind errors can be treated as failed test
191+
// expectations.
192+
err := vgOutput.E
193+
if err == nil && vgOutput.R != nil && len(vgOutput.R.Errors) > 0 {
194+
err = fmt.Errorf("detected %d memory errors", len(vgOutput.R.Errors))
195+
}
196+
197+
return resp.HeaderMap, output, err
198+
199+
}
200+
108201
// resultOrError is a poor man's sum type.
109202
type resultOrError struct {
110203
R *valgrind.Report

0 commit comments

Comments
 (0)