Feature/l2docs - #216
Open
nhanasi wants to merge 15 commits into
Open
Conversation
added 3 commits
May 20, 2026 10:35
Code Coverage Summary |
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds L2 functional-test coverage collection and reporting for remotedebugger, including gcov/lcov instrumentation and a generated L2 coverage report to document feature↔test mapping and gaps.
Changes:
- Enable gcov counter dumping on daemon SIGTERM in coverage builds (
USECOV) and adjust functional-test teardown to prefer SIGTERM when in coverage mode. - Extend the L2 runner/build scripts to compile with coverage flags and generate an lcov/genhtml HTML report.
- Add an L2 test coverage markdown report and a repository skill document related to BDD feature generation/gap analysis.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/functional-tests/tests/helper_functions.py |
Switches kill behavior in coverage mode to allow gcov data collection. |
test/functional-tests/L2_Test_Coverage.md |
Adds a generated coverage/gap analysis report for L2 tests vs features. |
src/rrdMain.c |
Installs a SIGTERM handler (coverage builds) to dump gcov counters before exiting. |
run_l2.sh |
Adds lcov baseline/capture and generates an HTML coverage report for L2 runs. |
cov_build.sh |
Builds remotedebugger with --coverage flags and USECOV enabled. |
.github/skills/bdd-feature-generator/SKILL.md |
Adds documentation for a BDD feature generation/gap analysis skill. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+26
to
+28
| #ifdef USECOV | ||
| #include <signal.h> | ||
| #include <stdlib.h> |
Comment on lines
+31
to
+36
| static void rrd_gcov_sigterm_handler(int sig) | ||
| { | ||
| (void)sig; | ||
| __gcov_dump(); | ||
| _exit(0); | ||
| } |
Comment on lines
57
to
+59
| def kill_rrd(signal: int=9): | ||
| if os.environ.get("RRD_COVERAGE_MODE") and signal == 9: | ||
| signal = 15 |
| | Metric | Count | | ||
| |---|:---:| | ||
| | Feature files | 21 | | ||
| | Feature scenarios | 90 | |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/rrdMain.c:35
- _exit() is used but <unistd.h> is not included. This can fail the build under stricter warning/error settings. Since <stdlib.h> is already included, using the standard _Exit() avoids the missing declaration.
_exit(0);
run_l2.sh:107
- The coverage capture/report generation steps don’t check exit codes, so failures can go unnoticed and still print a report path. Adding explicit failure handling helps make CI results reliable.
lcov --capture --directory "$(pwd)/src" \
--output-file "$COV_DIR/coverage_test.info" --rc lcov_branch_coverage=1
lcov --add-tracefile "$COV_DIR/coverage_base.info" \
--add-tracefile "$COV_DIR/coverage_test.info" \
--output-file "$COV_DIR/coverage_merged.info" --rc lcov_branch_coverage=1
test/functional-tests/tests/helper_functions.py:61
- The kill_rrd() parameter name shadows the imported signal module, and the function resolves the PID twice and shells out with shell=True. This makes the behavior harder to read and can race if the PID changes between calls. Consider using signal constants, capturing the PID once, and using os.kill() instead of a shell command.
def kill_rrd(signal: int=9):
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
print(f"Received Signal to kill remotedebugger {signal} with pid {get_pid('remotedebugger')}")
resp = subprocess.run(f"kill -{signal} {get_pid('remotedebugger')}", shell=True, capture_output=True)
run_l2.sh:70
- The coverage baseline capture commands don’t check for failure; if lcov errors (e.g., missing counters or permission issues), the script will continue and later steps may produce misleading output. Consider failing fast on lcov errors.
This issue also appears on line 103 of the same file.
# lcov baseline — capture zero counters before any test runs
lcov --zerocounters --directory "$(pwd)/src"
lcov --capture --initial --directory "$(pwd)/src" \
--output-file "$COV_DIR/coverage_base.info" --rc lcov_branch_coverage=1
added 2 commits
August 10, 2026 15:33
…_debugger into feature/l2docs
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/rrdMain.c:29
_exit(0)is used in theUSECOVSIGTERM handler, but<unistd.h>is not included, which can cause an implicit declaration warning/error (often fatal under-Werror).
#ifdef USECOV
#include <signal.h>
#include <stdlib.h>
extern void __gcov_dump(void);
test/functional-tests/tests/helper_functions.py:61
kill_rrd()callsget_pid()twice and usessubprocess.run(..., shell=True)with string interpolation. This can race (PID can change between calls) andshell=Trueis unnecessary here; passing an argv list is safer and avoids shell parsing issues ifpidofreturns multiple PIDs.
def kill_rrd(signal: int=9):
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
print(f"Received Signal to kill remotedebugger {signal} with pid {get_pid('remotedebugger')}")
resp = subprocess.run(f"kill -{signal} {get_pid('remotedebugger')}", shell=True, capture_output=True)
.github/workflows/L2-tests.yml:8
- This workflow commits
test/functional-tests/L2_Test_Coverage.mdback todeveloponpush. That bot commit will trigger this workflow again (extra, redundant run; and potentially a loop if metrics change). Add apaths-ignorefor this generated file on thepushtrigger (or otherwise gate bot pushes).
push:
branches: [ develop ]
Comment on lines
+25
to
+27
| COV_DIR="/tmp/l2_coverage" | ||
|
|
||
| export RRD_COVERAGE_MODE=1 |
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/rrdMain.c:36
- rrdMain.c uses _exit(0) in the USECOV SIGTERM handler, but <unistd.h> is not included. With modern toolchains (often built with -Werror=implicit-function-declaration), this can break the build. Also <stdlib.h> is unused in this new block.
#ifdef USECOV
#include <signal.h>
#include <stdlib.h>
extern void __gcov_dump(void);
/* Flush gcov counters on SIGTERM so lcov captures L2 exercise data. */
static void rrd_gcov_sigterm_handler(int sig)
{
(void)sig;
__gcov_dump();
_exit(0);
}
.github/workflows/L2-tests.yml:139
- The commit step stages test/functional-tests/L2_Test_Coverage.md, but the coverage report file is located at test/functional-tests/docs/L2_Test_Coverage.md. As written, the workflow will never commit the updated file.
cd remote_debugger
git config user.name "github-actions[bot]"
git config user.email "github-actions[bot]@users.noreply.github.com"
git add test/functional-tests/L2_Test_Coverage.md
git diff --cached --quiet || git commit -m "ci: update L2 lcov coverage metrics [skip ci]"
git push
test/functional-tests/tests/helper_functions.py:62
- kill_rrd() builds a shell command with shell=True. Even though the inputs are expected to be safe, using the shell here is avoidable and makes it easier for malformed PID output to become a command-line injection or cause hard-to-debug failures (e.g., multiple PIDs). Consider using os.kill() with parsed PIDs, and avoid magic numbers by using signal.SIGKILL/SIGTERM (also renames the parameter so it doesn't shadow the signal module).
def kill_rrd(signal: int=9):
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
print(f"Received Signal to kill remotedebugger {signal} with pid {get_pid('remotedebugger')}")
resp = subprocess.run(f"kill -{signal} {get_pid('remotedebugger')}", shell=True, capture_output=True)
print(resp.stdout.decode('utf-8'))
.github/workflows/L2-tests.yml:127
- The workflow updates L2_Test_Coverage.md using a path that doesn't exist in the repo (the file is under test/functional-tests/docs/). Also, the lcov metrics block replacement assumes markers exist; if they don't, nothing will be inserted. Consider switching to the correct path and inserting the block after the "Test Coverage Summary" header when markers are absent.
This issue also appears on line 134 of the same file.
path = 'remote_debugger/test/functional-tests/L2_Test_Coverage.md'
content = open(path).read()
content = re.sub(r'<!-- lcov-metrics-start -->.*?<!-- lcov-metrics-end -->', new_block, content, flags=re.DOTALL)
content = re.sub(r'\*\*Generated:\*\* \S+', f'**Generated:** {today}', content)
open(path, 'w').write(content)
.github/workflows/L2-tests.yml:133
- This workflow pushes a commit back to develop. That push will retrigger the workflow; while the current script may often result in "no changes", it's still safer to prevent the bot-authored push from attempting another push (and to avoid accidental push loops if the generated content changes each run). Add a github.actor guard to the commit step.
- name: Commit updated L2_Test_Coverage.md
if: github.event_name == 'push'
run: |
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/rrdMain.c:29
_exit(0)is used underUSECOV, but<unistd.h>is not included. On C99+ this can fail to compile due to an implicit declaration (often treated as an error with-Werror).
#ifdef USECOV
#include <signal.h>
#include <stdlib.h>
extern void __gcov_dump(void);
src/rrdMain.c:36
- The SIGTERM handler calls
__gcov_dump(), which is not guaranteed to be async-signal-safe. Calling non-async-signal-safe functions from a signal handler is undefined behavior and can deadlock/corrupt state, especially in a multi-threaded process.
static void rrd_gcov_sigterm_handler(int sig)
{
(void)sig;
__gcov_dump();
_exit(0);
}
test/functional-tests/tests/helper_functions.py:61
- The
kill_rrdparameter namesignalshadows the importedsignalmodule, andget_pid('remotedebugger')is executed twice (the PID could change between the log line and the kill command). Rename the parameter and store the PID once before printing/executing.
def kill_rrd(signal: int=9):
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
print(f"Received Signal to kill remotedebugger {signal} with pid {get_pid('remotedebugger')}")
resp = subprocess.run(f"kill -{signal} {get_pid('remotedebugger')}", shell=True, capture_output=True)
Comment on lines
+9
to
+10
| permissions: | ||
| contents: write |
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/rrdMain.c:28
_exit(0)is used in the SIGTERM handler but<unistd.h>is not included, which can cause an implicit declaration warning/error depending on compiler flags. Include<unistd.h>underUSECOV.
#include <signal.h>
#include <stdlib.h>
.github/workflows/L2-tests.yml:10
permissions: contents: writeapplies to bothpull_requestandpushtriggers. Since write access is only needed for the push-only commit step, consider splitting the workflow into separate jobs (or a separate workflow) so PR runs keepcontents: readto reduce token permissions.
permissions:
contents: write
test/functional-tests/tests/helper_functions.py:62
- The parameter name
signalshadows the importedsignalmodule in this file, which makes the newRRD_COVERAGE_MODElogic harder to read and prevents use ofsignal.SIG*constants. Rename the parameter (e.g.,sig) and usesignal.SIGKILL/signal.SIGTERMinstead of magic numbers.
def kill_rrd(signal: int=9):
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
print(f"Received Signal to kill remotedebugger {signal} with pid {get_pid('remotedebugger')}")
resp = subprocess.run(f"kill -{signal} {get_pid('remotedebugger')}", shell=True, capture_output=True)
src/rrdMain.c:155
- Using
signal()is less portable/reliable thansigaction()(semantics differ across platforms and can be reset unexpectedly). Prefersigaction(SIGTERM, ...)for predictable handler installation.
#ifdef USECOV
signal(SIGTERM, rrd_gcov_sigterm_handler);
#endif
.github/workflows/L2-tests.yml:177
- The generated markdown string is indented inside the Python heredoc; those leading spaces will be written to
L2_Coverage.mdand can cause GitHub to render most of the report as a code block. Build the triple-quoted string without leading indentation.
report = f"""# Remote Debugger L2 Coverage Report
**Generated:** {datetime.date.today()}
## Coverage Metrics
added 2 commits
August 12, 2026 15:17
…_debugger into feature/l2docs
Code Coverage Summary |
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/rrdMain.c:36
_exit(0)is used inside the USECOV SIGTERM handler, butrrdMain.cdoes not include<unistd.h>, which can cause an implicit declaration / build failure depending on compiler flags. Since<stdlib.h>is already included here,_Exit(0)avoids the missing header and is standard C.
static void rrd_gcov_sigterm_handler(int sig)
{
(void)sig;
__gcov_dump();
_exit(0);
}
test/functional-tests/generate_l2_coverage_report.py:430
- If the recommendations section is renumbered to "## 6", the appendix heading should also be renumbered to keep the generated document's section numbers sequential and unique.
'---',
'',
'## 7. Appendix: File Inventory',
'',
'### Feature Files',
.github/workflows/L2-tests.yml:11
- The workflow grants
contents: writefor all runs (includingpull_request). Since the only step that needs write access is the conditional "Commit L2_Coverage.md" onpush, consider splitting the push-only publishing into a separate job/workflow withcontents: write, and keep the PR test job atcontents: readto reduce the blast radius of a compromised dependency/action.
push:
branches: [ develop ]
permissions:
contents: write
test/functional-tests/generate_l2_coverage_report.py:355
generate()relies on a module-levelfeaturesglobal when listing orphan features, which makes the function non-self-contained and easy to break ifgenerate()is reused (and it currently depends onmain()having set a global). You can derive the scenario count frompairsinstead and avoid the global dependency.
for ff in orphan_feats:
sc = features[ff]
lines.append(f'| `{ff}` | {sc} | **Missing test file** |')
test/functional-tests/generate_l2_coverage_report.py:470
global featuresis unnecessary here (and the comment is inaccurate—build_mapping()does not close overfeatures). Keeping this as a local variable avoids accidental cross-call state.
global features # used in build_mapping closure for orphan label
features = scan_features(args.features_dir)
test/functional-tests/generate_l2_coverage_report.py:149
- Section numbering in the generated markdown is inconsistent: this static block is labeled "## 5" but is emitted after the "## 5. Scenario-to-Test Gap Analysis" section, causing duplicate section numbers.
This issue also appears on line 426 of the same file.
_RECOMMENDATIONS = """\
## 6. Gap Recommendations
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/functional-tests/tests/helper_functions.py:59
RRD_COVERAGE_MODEis treated as truthy for any non-empty value (e.g., "0"), which could unexpectedly switch SIGKILL (9) to SIGTERM (15). It’s safer to explicitly check the intended value.
def kill_rrd(signal: int=9):
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
src/rrdMain.c:29
_exit(0)is used but<unistd.h>isn’t included, which can cause an implicit declaration warning/error depending on compiler flags. Include<unistd.h>whenUSECOVis enabled.
#ifdef USECOV
#include <signal.h>
#include <stdlib.h>
extern void __gcov_dump(void);
.github/workflows/L2-tests.yml:10
- Global workflow
contents: writepermission is granted for bothpull_requestandpushevents, but only the push-only "Commit L2_Coverage.md" step needs it. Keeping write permission enabled on PR runs unnecessarily increases the blast radius if a PR can influence the job execution.
permissions:
contents: write
run_l2.sh:28
- The script now depends on
lcov/genhtmlfor generating coverage artifacts, but it doesn’t fail fast on errors. Withoutset -e, a failed coverage capture can be silently ignored and the job may still report success.
RESULT_DIR="/tmp/l2_test_report"
STATIC_PROFILE_DIR="/etc/rrd"
OUTPUT_DIR="/tmp/rrd"
LIB_DIR="/lib/rdk"
COV_DIR="/tmp/l2_coverage"
export RRD_COVERAGE_MODE=1
test/functional-tests/generate_l2_coverage_report.py:26
sysis imported but not used anywhere in this script; removing it avoids unnecessary lint noise and keeps the module focused.
import argparse
import datetime
import os
import re
import sys
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/rrdMain.c:36
_exit()is used in the SIGTERM handler but<unistd.h>is not included, which can cause a build failure (implicit declaration) on modern C compilers. Also<stdlib.h>appears unused in this block.
static void rrd_gcov_sigterm_handler(int sig)
{
(void)sig;
__gcov_dump();
_exit(0);
}
test/functional-tests/generate_l2_coverage_report.py:471
featuresis declaredglobalhere, but it’s only needed becausegenerate()reads it implicitly. Ifgenerate()is updated to avoid the global, thisglobal featuresline becomes unnecessary and misleading.
global features # used in build_mapping closure for orphan label
features = scan_features(args.features_dir)
tests = scan_tests(args.tests_dir)
.github/workflows/L2-tests.yml:10
permissions: contents: writeapplies to pull_request runs too. That gives the workflow write access to the repo when executing PR code, which is a security risk (especially for PRs from branches within the same repo). Consider scoping write permissions to a push-only job (or splitting into separate PR/push jobs) so PR runs use read-only permissions.
permissions:
contents: write
test/functional-tests/generate_l2_coverage_report.py:356
- This loop relies on a global
featuresdict (set inmain()), creating a hidden dependency insidegenerate(). Since the scenario count is already available viapairs, this can be computed locally and the global can be avoided.
This issue also appears on line 469 of the same file.
for ff in orphan_feats:
sc = features[ff]
lines.append(f'| `{ff}` | {sc} | **Missing test file** |')
lines.append('')
Code Coverage Summary |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/rrdMain.c:28
_exit(0)is used in theUSECOVSIGTERM handler but<unistd.h>is not included, which can cause an implicit declaration warning/error depending on toolchain flags.
#ifdef USECOV
#include <signal.h>
#include <stdlib.h>
src/rrdMain.c:35
- The SIGTERM handler calls
__gcov_dump()from a signal context in a multi-threaded process.__gcov_dump()is not guaranteed to be async-signal-safe, and SIGTERM may be delivered to any thread, so this can deadlock/crash under load. A safer pattern is to block SIGTERM in worker threads and have the main threadsigwait()/handle shutdown, then call__gcov_dump()from normal control flow before exiting.
static void rrd_gcov_sigterm_handler(int sig)
{
(void)sig;
__gcov_dump();
_exit(0);
.github/workflows/L2-tests.yml:10
contents: writeis granted at workflow scope, even though the only write action is a conditional commit/push step onpushevents. Consider scoping write permissions to a separate job that only runs onpush(or using job-level permissions) so PR runs keep least-privilege permissions.
permissions:
contents: write
test/functional-tests/tests/helper_functions.py:61
kill_rrduses a parameter namedsignal, which shadows the importedsignalmodule and forces use of magic numbers (9/15). Renaming the parameter and usingsignal.SIGKILL/signal.SIGTERMimproves readability and avoids accidental module shadowing; while here, caching the pid avoids callingget_pid()twice.
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
test/functional-tests/generate_l2_coverage_report.py:25
sysis imported but never used in this script. Unused imports can hide real dependencies and may trip linting in CI.
import argparse
import datetime
import os
import re
import sys
.github/workflows/L2-tests.yml:7
- The workflow trigger was narrowed from
developto onlyfeature/l2docs. That prevents this workflow from running on the main development branch, which is usually not intended for CI coverage/testing workflows.
on:
pull_request:
branches: [ feature/l2docs ]
push:
branches: [ feature/l2docs ]
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/rrdMain.c:29
rrd_gcov_sigterm_handler()calls_exit(0)butrrdMain.cdoes not include<unistd.h>, which can cause an implicit declaration build error (common with C99+ and/or-Werror).
#ifdef USECOV
#include <signal.h>
#include <stdlib.h>
extern void __gcov_dump(void);
test/functional-tests/tests/helper_functions.py:61
kill_rrdcurrently uses magic numbers (9/15), shadows the importedsignalmodule with its parameter name, and callsget_pid()twice. This makes the intent harder to follow and can also produce an invalidkillinvocation when no PID is running. Consider usingsignal.SIGKILL/signal.SIGTERM, renaming the parameter, capturing the PID once, and avoidingshell=True.
def kill_rrd(signal: int=9):
if os.environ.get("RRD_COVERAGE_MODE") and signal == 9:
signal = 15
print(f"Received Signal to kill remotedebugger {signal} with pid {get_pid('remotedebugger')}")
resp = subprocess.run(f"kill -{signal} {get_pid('remotedebugger')}", shell=True, capture_output=True)
test/functional-tests/generate_l2_coverage_report.py:25
generate_l2_coverage_report.pyimportssysbut does not use it anywhere, which will trigger lint warnings and adds noise.
import argparse
import datetime
import os
import re
import sys
test/functional-tests/generate_l2_coverage_report.py:356
generate()relies on a globalfeaturesvariable when rendering orphan features, even though all needed data is already available viapairs. This hidden dependency makes the function harder to reuse/test and can break ifgenerate()is called from anywhere exceptmain().
for ff in orphan_feats:
sc = features[ff]
lines.append(f'| `{ff}` | {sc} | **Missing test file** |')
.github/workflows/L2-tests.yml:8
- This workflow is now restricted to
feature/l2docsfor bothpull_requestandpush. In this repo, other CI workflows targetdevelop(e.g..github/workflows/L1-Test.yml:4-6,native_full_build.yml:3-6), so this change will prevent L2 tests from running on normal PRs intodevelop. Consider includingdevelop(and any other supported base branches) in the trigger list.
on:
pull_request:
branches: [ feature/l2docs ]
push:
branches: [ feature/l2docs ]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.