CHORE: Exclude diagnostic LOG statements from coverage metrics - #556
Conversation
Sorry, something went wrong.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.7%
mssql_python.row.py: 70.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.1%
mssql_python.__init__.py: 77.3%
mssql_python.pybind.connection.connection.cpp: 77.5%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 85.4%
mssql_python.logging.py: 85.5%🔗 Quick Links
|
Sorry, something went wrong.
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
A revamped take on the LOG-coverage gap
I called this exact gap out in the logging framework PR a few months ago: #312 (comment). at the time I rejected the LCOV markers approach for the same reasons that show up here: 150+ manual changes, clutters the codebase, hides executable code paths. coming back to it now I think there's a third option we didn't consider then that gets the same metric lift without any of those costs.
the 600 LCOV_EXCL_LINE markers and the python filter in generate_codecov.sh can be replaced with one tiny pre-build step plus one extra lcov flag. same coverage number, cleaner source tree, no helper scripts.
ran it end-to-end on macos arm64. full pytest suite including the logging integration tests:
PR's machinery: 3916/5079 = 77.10% proposed flow: 3912/5052 = 77.43%
1733/1733 tests pass. logging still works at runtime, LoggerBridge gets normal coverage, nothing about behavior changes.
the proposal
- add a small script (scripts/join_logs_for_coverage.py, ~50 lines) that joins multi-line LOG calls onto single lines. only used in the codecov build, never committed source changes.
- in mssql_python/pybind/build.sh codecov mode, snapshot the source, run the join, build, restore on EXIT via trap (~12 lines).
- in generate_codecov.sh, swap the embedded python filter for --omit-lines '\bLOG[A-Z_]*\(' on the existing lcov -a line.
joining preserves runtime behavior because adjacent string literals concatenate at compile time. the .so built from joined source is bit-for-bit equivalent to one built from the original.
what gets deleted
- all 600 // LCOV_EXCL_LINE markers from the cpp files
- add_lcov_exclusions.py and fix_multiline_log_exclusions.py at repo root
- the embedded ~90-line PYTHON_FILTER block in generate_codecov.sh
tests/test_021_coverage_edge_cases.py and .gitignore additions stay.
small bonus
the helper scripts use 'LOG(' in line which misses LOG_ERROR( and LOG_WARNING(. 11 sites currently unmarked (9 LOG_ERROR + 1 LOG_WARNING + their continuations). the proposed regex catches them. that's the +0.33pp.
Sorry, something went wrong.
Replace manual LCOV_EXCL_LINE markers with lcov's built-in --omit-lines flag.
This approach is cleaner, more maintainable, and catches all LOG variants.
Changes:
- Add eng/scripts/join_logs_for_coverage.py to join multi-line LOG calls during coverage builds
- Modify build.sh to temporarily join LOG statements in codecov mode
- Replace Python filter in generate_codecov.sh with --omit-lines '\bLOG[A-Z_]*\('
- Update .gitignore to exclude local development scripts
Benefits:
- No source code clutter (600+ markers removed)
- Catches LOG_ERROR, LOG_WARNING, and all LOG variants
- Cleaner, more maintainable approach
- Source files remain unchanged in repository
Addresses review feedback from @bewithgaurav on PR #556
There was a problem hiding this comment.
Pull request overview
The PR is described as adding // LCOV_EXCL_LINE markers to ~284 LOG() statements across the C++ sources, but what is actually committed is a different mechanism: during coverage builds, source files are temporarily rewritten to collapse multi-line LOG(...) calls onto a single line, then lcov --omit-lines is used with a \bLOG[A-Z_]*\( regex to drop those lines from the merged report. Local helper scripts (including add_lcov_exclusions.py) and coverage artifacts are also added to .gitignore.
Changes:
- build.sh (coverage mode): backs up .cpp/.hpp files, invokes eng/scripts/join_logs_for_coverage.py to collapse multi-line LOG calls, and restores originals via an EXIT trap.
- generate_codecov.sh: passes --omit-lines '\bLOG[A-Z_]*\(' to lcov during the merge step and tweaks the surrounding comments.
- New eng/scripts/join_logs_for_coverage.py that walks *.cpp/*.hpp and joins multi-line LOG macro invocations using naive paren counting.
- .gitignore updated with coverage outputs (*.profraw, *.profdata, *.info, htmlcov/, etc.) and several local-only helper scripts.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| mssql_python/pybind/build.sh | Adds coverage-mode pre-step that backs up/rewrites C++ source and restores it on exit. |
| generate_codecov.sh | Adds --omit-lines regex to lcov merge and updates comments. |
| eng/scripts/join_logs_for_coverage.py | New helper that joins multi-line LOG() calls onto one line. |
| .gitignore | Ignores coverage artifacts and several local experimental scripts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
Replace manual LCOV_EXCL_LINE markers with cleaner built-in lcov filtering.
This approach uses lcov's native exclusion mechanism and is more maintainable.
Changes:
- Add eng/scripts/join_logs_for_coverage.py to join multi-line LOG calls during coverage builds
- Modify build.sh to temporarily join LOG statements in codecov mode with automatic restore
- Use lcov --rc lcov_excl_line='\bLOG[A-Z_]*\s*\(' to exclude LOG macros from coverage
- Add llvm-cov ignore pattern for build/_deps/ (vendored simdutf sources from PR #526)
- Add lcov --remove for build/_deps/ as defense-in-depth (from PR #579)
- Update .gitignore to exclude local development scripts
Benefits:
- No source code clutter (600+ markers not needed)
- Catches all LOG variants (LOG_ERROR, LOG_WARNING, etc.)
- Excludes vendored third-party dependencies from coverage metrics
- Cleaner, more maintainable approach using lcov native features
- Source files remain unchanged in repository
Addresses review feedback from @bewithgaurav on PR #556
Includes changes from PR #579 to fix simdutf coverage pollution
…ormat Critical fixes: 1. Fixed restore timing: Source files now restored AFTER llvm-cov/lcov analysis completes 2. Fixed backup format mismatch: generate_codecov.sh now correctly extracts tar.gz backup 3. Removed duplicate cleanup section in generate_codecov.sh Existing safeguards in place: - join_logs_for_coverage.py has max_lines=20 limit to prevent runaway joins - Unbalanced parentheses detection with warning messages - tar.gz backup preserves directory structure correctly - Error handling with automatic restore on join failure Addresses Copilot automated code review feedback
Critical fix for Copilot review comment (Medium priority): - Switched from naive parenthesis counting to semicolon-based joining - Prevents issues with unbalanced parens in C++ string literals - More reliable for C-style statements that always end with semicolon Impact on coverage: - Ensures ALL multi-line LOG statements are joined properly - Previously, LOGs with unbalanced parens in strings failed to join - Their continuation lines were still counted toward coverage - This fix should improve coverage from 80% closer to 81% Addresses Copilot automated code review feedback
Critical improvement over semicolon-based approach:
- Implements proper C++ tokenizer that understands syntax
- Correctly handles string literals, char literals, comments
- Avoids corruption from semicolons/parens in strings
Examples now handled correctly:
- LOG(""SQL: SELECT *; WHERE"", x); - semicolon in string
- LOG(""unbalanced ("", x); - unbalanced paren in string
- LOG(')', code); - closing paren as character literal
- LOG(""msg"", x); // comment with ) - comment with paren
This is the only robust way to parse C++ syntax.
Addresses Copilot code review suggestion (option a)
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
need one more refinement w.r.t code coverage and minor fixes on the join logs script, rest is good
Sorry, something went wrong.
…iltering Implements 3 critical fixes from code review: 1. Fix lcov LOG exclusion (generate_codecov.sh): - Changed from --rc lcov_excl_line to --omit-lines - --rc requires --filter region flag to work (wasn't applied) - Measured coverage improvement: 75.42% -> 77.22% - --omit-lines directly excludes matching lines 2. Include .h header files (join_logs_for_coverage.py): - Changed '*.hpp' to '*.h*' to match both .h and .hpp - Catches files like ddbc_bindings.h with LOG statements - Prevents future multi-line LOGs in headers from being missed 3. Skip build directory (join_logs_for_coverage.py): - Added guard: if 'build' in filepath.parts: continue - Avoids processing cmake-generated files like CMakeCXXCompilerId.cpp - Prevents unnecessary processing of build artifacts All changes verified as 100% correct per reviewer feedback.
# Conflicts: # generate_codecov.sh
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
lgtm, thanks!
Sorry, something went wrong.
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
just saw a corner case and left a minor comment
can choose to fix here or later, not p0
Sorry, something went wrong.
Addresses bewithgaurav review comment: - The join script processes *.cpp + *.h* (includes .h and .hpp) - But tar backup only caught *.cpp + *.hpp (missed .h files) - Changed find pattern from "*.hpp" to "*.h*" to match joiner Impact: - Ensures .h files like ddbc_bindings.h are backed up - Prevents permanent corruption if multi-line LOGs added to .h files - Currently zero impact (existing .h files have only single-line LOGs) - Future-proofs against .h file modifications not being restored This keeps backup and processing patterns synchronized.
Work Item / Issue Reference
Summary
This pull request introduces a new workflow for improving code coverage reporting by automatically joining multi-line LOG() macro calls into single lines before coverage analysis. This makes it easier to exclude logging statements from LCOV coverage results without modifying the original source code. The changes include a new helper script, updates to the build script to use this helper during coverage builds, and adjustments to the coverage merge process to exclude LOG statements.
Coverage build improvements:
Coverage reporting adjustments: