SteNicholas commented on code in PR #489: URL: https://github.com/apache/paimon-cpp/pull/489#discussion_r4440166933
########## test/generate_coverage.sh: ########## @@ -20,10 +20,25 @@ set -e cd $(dirname "$0")/../ mkdir -p build && cd build -cmake ../ -DCMAKE_BUILD_TYPE=Debug -DPAIMON_BUILD_TESTS=ON -DPAIMON_USE_ASAN=ON -DPAIMON_GENERATE_COVERAGE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON +cmake ../ -DCMAKE_BUILD_TYPE=Debug -DPAIMON_BUILD_TESTS=ON -DPAIMON_USE_ASAN=ON -DPAIMON_GENERATE_COVERAGE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ + -DPAIMON_ENABLE_AVRO=ON -DPAIMON_ENABLE_ORC=ON \ + -DPAIMON_ENABLE_MOSAIC=ON -DPAIMON_ENABLE_LANCE=ON -DPAIMON_ENABLE_VORTEX=ON make -j make test lcov --capture --directory src/paimon --directory test --output-file coverage.info +# Keep only project implementation code in the report: drop system headers, +# third-party dependencies from the build tree and third_party/, and test +# sources (*_test.cpp, test helpers, mocks). +lcov --remove coverage.info \ + '/usr/*' \ + '*/build*/*' \ Review Comment: [P2] Anchor the build exclusion to the actual build directory This wildcard matches any ancestor directory beginning with `build`, including a checkout at `/builds/paimon-cpp` or `/home/builder/paimon-cpp`. Consequently, it also removes legitimate records such as `/builds/paimon-cpp/src/paimon/status.cpp` and `/builds/paimon-cpp/include/paimon/result.h`. I reproduced an empty-trace failure with the former layout, even after ignoring unused-pattern errors. Please scope this exclusion to the absolute build directory, or positively extract the absolute `src/paimon` and `include/paimon` roots before excluding test sources. ########## test/generate_coverage.sh: ########## @@ -20,10 +20,25 @@ set -e cd $(dirname "$0")/../ mkdir -p build && cd build -cmake ../ -DCMAKE_BUILD_TYPE=Debug -DPAIMON_BUILD_TESTS=ON -DPAIMON_USE_ASAN=ON -DPAIMON_GENERATE_COVERAGE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON +cmake ../ -DCMAKE_BUILD_TYPE=Debug -DPAIMON_BUILD_TESTS=ON -DPAIMON_USE_ASAN=ON -DPAIMON_GENERATE_COVERAGE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ + -DPAIMON_ENABLE_AVRO=ON -DPAIMON_ENABLE_ORC=ON \ + -DPAIMON_ENABLE_MOSAIC=ON -DPAIMON_ENABLE_LANCE=ON -DPAIMON_ENABLE_VORTEX=ON make -j make test lcov --capture --directory src/paimon --directory test --output-file coverage.info +# Keep only project implementation code in the report: drop system headers, +# third-party dependencies from the build tree and third_party/, and test +# sources (*_test.cpp, test helpers, mocks). +lcov --remove coverage.info \ + '/usr/*' \ + '*/build*/*' \ + '*/third_party/*' \ + '*_test.cpp' \ + '*/test/*' \ + '*/testing/*' \ + '*mock*' \ Review Comment: [P2] Make unused exclusion patterns nonfatal LCOV 2.x counts only the first matching exclusion for each source and treats unused patterns as errors. Here, `*_test.cpp` already removes the integration-test sources before `*/test/*` can match them, and `*/testing/*` removes the testing mocks before `*mock*` can match them. With a synthetic trace containing project sources, dependencies, unit/integration tests, helpers, and mocks, this exact command exits with code 25: `lcov: ERROR: 'exclude' pattern '*/test/*' is unused.` Because the script uses `set -e`, it stops before replacing `coverage.info` or generating HTML. Please make unused patterns nonfatal for this filtering step (for example, with `--ignore-errors unused` on LCOV 2.x). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
