Skip to content

Commit 07bd2bd

Browse files
committed
fix(instrument): emit coverage signal after statement execution
Move the injected pg_notify() call from before the tracked statement to after it so that coverage reflects successful execution rather than attempted execution. Previously, if a statement raised an exception caught by the caller, the signal had already been sent — recording false coverage. Terminal statements (RETURN, RAISE EXCEPTION, bare RAISE) keep the signal before the statement because they exit the current scope and any code placed after them would be unreachable. Non-fatal RAISE levels (NOTICE, WARNING, INFO, LOG, DEBUG) are correctly identified as non-terminal and receive after-execute signals. Add isTerminalSegment() helper that inspects the first meaningful token of a segment to distinguish terminal from non-terminal statements. Add tests: TestIsTerminalSegment (17 cases), TestInstrumentBody_CoverageAfterExecute (verifies ordering for assignments vs RETURN), TestInstrumentBody_RaiseExceptionBeforeNotify (verifies RAISE EXCEPTION vs RAISE NOTICE ordering).
1 parent a94a4b5 commit 07bd2bd

2 files changed

Lines changed: 206 additions & 7 deletions

File tree

internal/instrument/instrumenter.go

Lines changed: 74 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,13 @@ func instrumentBody(stmt *parser.Statement, filePath string, skipToBegin bool, n
126126
segStart := -1
127127

128128
// emitSegment checks the segment between segStart..segEnd for
129-
// executability and, if it qualifies, writes the gap + notify + segment
130-
// into instrumentedBody.
129+
// executability and, if it qualifies, writes the gap + signal + segment
130+
// (or segment + signal) into instrumentedBody.
131+
//
132+
// For terminal statements (RETURN, RAISE EXCEPTION) the signal is
133+
// emitted *before* the statement because nothing executes after them.
134+
// For all other statements the signal is emitted *after* the statement
135+
// so that coverage is recorded only on successful execution (B1 fix).
131136
emitSegment := func(segEnd int) {
132137
segText := bodyContent[segStart:segEnd]
133138
if !isExecutableSegment(segText) {
@@ -160,11 +165,29 @@ func instrumentBody(stmt *parser.Statement, filePath string, skipToBegin bool, n
160165
}
161166
}
162167

163-
// Write notify call, then the original segment text.
164-
fmt.Fprintf(&instrumentedBody, "%s%s pg_notify('pgcov', '%s');\n",
168+
notifyCall := fmt.Sprintf("%s%s pg_notify('pgcov', '%s');",
165169
indent, notifyCmd, strings.ReplaceAll(cp.SignalID, "'", "''"))
166-
instrumentedBody.WriteString(segText)
167-
lastWrittenPos = segEnd
170+
171+
if isTerminalSegment(segText) {
172+
// Terminal statements (RETURN, RAISE EXCEPTION) exit the
173+
// current scope — the signal must fire before the statement.
174+
fmt.Fprintf(&instrumentedBody, "%s\n", notifyCall)
175+
instrumentedBody.WriteString(segText)
176+
lastWrittenPos = segEnd
177+
} else {
178+
// Non-terminal statements: emit the signal after the
179+
// statement so coverage reflects successful execution.
180+
instrumentedBody.WriteString(segText)
181+
// Consume the semicolon that terminates this segment so
182+
// the notify call sits between statement and next gap.
183+
if segEnd < len(bodyContent) && bodyContent[segEnd] == ';' {
184+
instrumentedBody.WriteByte(';')
185+
lastWrittenPos = segEnd + 1
186+
} else {
187+
lastWrittenPos = segEnd
188+
}
189+
fmt.Fprintf(&instrumentedBody, "\n%s", notifyCall)
190+
}
168191
}
169192

170193
// Stream tokens one at a time – mirrors SplitStatements style.
@@ -251,6 +274,51 @@ func isExecutableSegment(segmentContent string) bool {
251274
return true
252275
}
253276

277+
// isTerminalSegment checks whether a segment starts with a terminal statement
278+
// (RETURN or RAISE EXCEPTION / bare RAISE) that exits the current scope.
279+
// NOTIFY calls placed after such statements would be unreachable.
280+
// Non-fatal RAISE levels (NOTICE, WARNING, INFO, LOG, DEBUG) are not terminal.
281+
func isTerminalSegment(segmentContent string) bool {
282+
sc := pglex.NewScanner(segmentContent)
283+
284+
// Find the first non-comment token.
285+
var first pglex.Token
286+
for {
287+
first = sc.Scan()
288+
if first.Type == pglex.EOF {
289+
return false
290+
}
291+
if first.Type != pglex.Comment {
292+
break
293+
}
294+
}
295+
296+
if first.Type == pglex.KReturn {
297+
return true
298+
}
299+
300+
if first.Type == pglex.KRaise {
301+
// RAISE is terminal unless followed by a non-fatal level.
302+
for {
303+
tok := sc.Scan()
304+
if tok.Type == pglex.EOF {
305+
return true // bare RAISE; — re-raise in exception handler
306+
}
307+
if tok.Type == pglex.Comment {
308+
continue
309+
}
310+
switch tok.Type {
311+
case pglex.KNotice, pglex.KWarning, pglex.KInfo, pglex.KLog, pglex.KDebug:
312+
return false
313+
default:
314+
return true // RAISE EXCEPTION, RAISE 'message', etc.
315+
}
316+
}
317+
}
318+
319+
return false
320+
}
321+
254322
// getIndentation returns the leading whitespace of a line.
255323
func getIndentation(line string) string {
256324
return line[:len(line)-len(strings.TrimLeft(line, " \t"))]

internal/instrument/plpgsql_ast_test.go

Lines changed: 132 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,8 @@ $$ LANGUAGE plpgsql;`
103103
}
104104
}
105105

106-
// Verify PERFORM statements are injected before the executable lines
106+
// Verify PERFORM statements are injected after the executable lines (coverage-after-execute),
107+
// except for terminal statements (RETURN) which keep PERFORM before.
107108
// Note: After instrumentation, line numbers shift due to inserted PERFORM statements
108109
// We just verify that PERFORM statements exist for each coverage point
109110
for _, cp := range instrumented.Locations {
@@ -236,3 +237,133 @@ $$ LANGUAGE plpgsql;`
236237
}
237238
t.Logf("Coverage points: %d (may be 0 for malformed SQL)", len(instrumented.Locations))
238239
}
240+
241+
func TestIsTerminalSegment(t *testing.T) {
242+
tests := []struct {
243+
name string
244+
segment string
245+
terminal bool
246+
}{
247+
{"RETURN value", "RETURN a + b", true},
248+
{"RETURN bare", "RETURN", true},
249+
{"RAISE EXCEPTION", "RAISE EXCEPTION 'error'", true},
250+
{"RAISE with string (default EXCEPTION)", "RAISE 'something went wrong'", true},
251+
{"RAISE bare re-raise", "RAISE", true},
252+
{"RAISE NOTICE", "RAISE NOTICE 'hello'", false},
253+
{"RAISE WARNING", "RAISE WARNING 'warn'", false},
254+
{"RAISE INFO", "RAISE INFO 'info'", false},
255+
{"RAISE LOG", "RAISE LOG 'log'", false},
256+
{"RAISE DEBUG", "RAISE DEBUG 'debug'", false},
257+
{"assignment", "discount_rate := 0.20", false},
258+
{"IF block", "IF x > 0 THEN\n y := 1", false},
259+
{"PERFORM", "PERFORM some_function()", false},
260+
{"SELECT", "SELECT 1 INTO result", false},
261+
{"comment then RETURN", "-- comment\nRETURN 42", true},
262+
{"comment then assignment", "-- comment\nx := 1", false},
263+
{"empty", "", false},
264+
}
265+
266+
for _, tt := range tests {
267+
t.Run(tt.name, func(t *testing.T) {
268+
got := isTerminalSegment(tt.segment)
269+
if got != tt.terminal {
270+
t.Errorf("isTerminalSegment(%q) = %v, want %v", tt.segment, got, tt.terminal)
271+
}
272+
})
273+
}
274+
}
275+
276+
func TestInstrumentBody_CoverageAfterExecute(t *testing.T) {
277+
// Verify that for non-terminal statements, NOTIFY comes after the statement,
278+
// and for RETURN, NOTIFY comes before.
279+
sql := `CREATE OR REPLACE FUNCTION example(x INT)
280+
RETURNS INT AS $$
281+
BEGIN
282+
x := x + 1;
283+
RETURN x;
284+
END;
285+
$$ LANGUAGE plpgsql;`
286+
287+
stmts := parser.ParseStatements(sql)
288+
if len(stmts) == 0 {
289+
t.Fatal("ParseStatements() returned no statements")
290+
}
291+
292+
instrumentedSQL, coveragePoints := instrumentBody(stmts[0], "test.sql", true, "PERFORM")
293+
if len(coveragePoints) != 2 {
294+
t.Fatalf("expected 2 coverage points, got %d", len(coveragePoints))
295+
}
296+
297+
// The assignment "x := x + 1" should have NOTIFY *after* it.
298+
assignSignal := coveragePoints[0].SignalID
299+
assignNotify := fmt.Sprintf("PERFORM pg_notify('pgcov', '%s');", assignSignal)
300+
assignIdx := strings.Index(instrumentedSQL, assignNotify)
301+
assignStmtIdx := strings.Index(instrumentedSQL, "x := x + 1")
302+
if assignIdx < 0 || assignStmtIdx < 0 {
303+
t.Fatal("could not find assignment or its notify in instrumented SQL")
304+
}
305+
if assignIdx <= assignStmtIdx {
306+
t.Errorf("assignment: NOTIFY at %d should come after statement at %d (coverage-after-execute)", assignIdx, assignStmtIdx)
307+
}
308+
309+
// The RETURN should have NOTIFY *before* it (terminal statement).
310+
returnSignal := coveragePoints[1].SignalID
311+
returnNotify := fmt.Sprintf("PERFORM pg_notify('pgcov', '%s');", returnSignal)
312+
returnIdx := strings.Index(instrumentedSQL, returnNotify)
313+
returnStmtIdx := strings.Index(instrumentedSQL, "RETURN x")
314+
if returnIdx < 0 || returnStmtIdx < 0 {
315+
t.Fatal("could not find RETURN or its notify in instrumented SQL")
316+
}
317+
if returnIdx >= returnStmtIdx {
318+
t.Errorf("RETURN: NOTIFY at %d should come before statement at %d (terminal statement)", returnIdx, returnStmtIdx)
319+
}
320+
321+
t.Log(instrumentedSQL)
322+
}
323+
324+
func TestInstrumentBody_RaiseExceptionBeforeNotify(t *testing.T) {
325+
// Use standalone RAISE statements so each is its own segment.
326+
sql := `CREATE OR REPLACE FUNCTION check_positive(x INT)
327+
RETURNS VOID AS $$
328+
BEGIN
329+
RAISE NOTICE 'checking value: %', x;
330+
RAISE EXCEPTION 'negative value: %', x;
331+
END;
332+
$$ LANGUAGE plpgsql;`
333+
334+
stmts := parser.ParseStatements(sql)
335+
if len(stmts) == 0 {
336+
t.Fatal("ParseStatements() returned no statements")
337+
}
338+
339+
instrumentedSQL, coveragePoints := instrumentBody(stmts[0], "test.sql", true, "PERFORM")
340+
if len(coveragePoints) != 2 {
341+
t.Fatalf("expected 2 coverage points, got %d", len(coveragePoints))
342+
}
343+
344+
// RAISE NOTICE is non-terminal — NOTIFY should come after it.
345+
raiseNoticeSignal := coveragePoints[0].SignalID
346+
raiseNoticeNotify := fmt.Sprintf("PERFORM pg_notify('pgcov', '%s');", raiseNoticeSignal)
347+
raiseNoticeIdx := strings.Index(instrumentedSQL, raiseNoticeNotify)
348+
raiseNoticeStmtIdx := strings.Index(instrumentedSQL, "RAISE NOTICE")
349+
if raiseNoticeIdx < 0 || raiseNoticeStmtIdx < 0 {
350+
t.Fatal("could not find RAISE NOTICE or its notify")
351+
}
352+
if raiseNoticeIdx <= raiseNoticeStmtIdx {
353+
t.Errorf("RAISE NOTICE: NOTIFY at %d should come after statement at %d", raiseNoticeIdx, raiseNoticeStmtIdx)
354+
}
355+
356+
// RAISE EXCEPTION is terminal — NOTIFY should come before it.
357+
raiseExcSignal := coveragePoints[1].SignalID
358+
raiseExcNotify := fmt.Sprintf("PERFORM pg_notify('pgcov', '%s');", raiseExcSignal)
359+
raiseExcIdx := strings.Index(instrumentedSQL, raiseExcNotify)
360+
raiseExcStmtIdx := strings.Index(instrumentedSQL, "RAISE EXCEPTION")
361+
if raiseExcIdx < 0 || raiseExcStmtIdx < 0 {
362+
t.Fatal("could not find RAISE EXCEPTION or its notify")
363+
}
364+
if raiseExcIdx >= raiseExcStmtIdx {
365+
t.Errorf("RAISE EXCEPTION: NOTIFY at %d should come before statement at %d", raiseExcIdx, raiseExcStmtIdx)
366+
}
367+
368+
t.Log(instrumentedSQL)
369+
}

0 commit comments

Comments
 (0)