Conversation
- Add googletest as git submodule in vendor/googletest - Replace TAP-based test infrastructure with googletest - Convert area tests to parameterized googletest format - Remove old tap.cpp and tap.hpp files - Update CMakeLists.txt to build with googletest instead of TAP 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add Google Benchmark as git submodule in vendor/benchmark - Replace custom timing infrastructure with Google Benchmark - Convert benchmark functions to use benchmark::State API - Create separate benchmarks for Earcut and Libtess2 algorithms - Add proper benchmark registration with meaningful names - Use benchmark::DoNotOptimize() to prevent result optimization - Add .claude directory to .gitignore 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Add -ffp-contract=off compiler flag to test targets to ensure consistent floating-point behavior across different CPU architectures. This prevents fused multiply-add (FMA) instructions from causing precision differences that lead to different triangulation results between x86_64 and arm64. The fix is applied only to test-related targets (fixtures library and tests executable) and does not affect library users or production builds. Resolves the issue where issue142 test produced 7 triangles on arm64 instead of the expected 4 triangles that pass on x86_64. References: GitHub issues #97 and #103, MySQL bug #82760 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
3c33d2a to
69c0ab2
Compare
Add a dedicated Coverage job that: - Uses g++ with --coverage flag for comprehensive coverage collection - Installs and configures lcov for coverage data processing - Filters coverage to focus on main library headers (include/*) - Excludes system headers, vendor files, tests, and build artifacts - Generates HTML coverage reports with titles and legends - Uploads results to Codecov for PR comments and online tracking - Provides downloadable coverage artifacts as backup The coverage job runs independently of main builds to avoid performance overhead while providing detailed coverage metrics and PR feedback. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add new Sanitizers job, testing with combined ASan+UBSan - Use clang++ compiler for better sanitizer support - Configure strict sanitizer options to abort on any detected issues - Enables detection of memory leaks, buffer overflows, and undefined behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Check all C++ files (*.hpp, *.cpp) in include/ and test/ directories - Fail CI if code is not formatted according to clang-format style - Generate and upload formatting patch as artifact when issues found - Show patch content in CI logs for easy review - Helps maintain consistent code style across the project 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
3407dbe to
df96ec8
Compare
There was a problem hiding this comment.
Pull Request Overview
A code formatting update that reformats test fixture data and migrates from a custom TAP testing framework to Google Test (gtest). The formatting changes improve readability by properly spacing coordinate arrays and constructor parameters across multiple lines.
- Migrated test framework from custom TAP to Google Test with proper test parametrization
- Reformatted large coordinate arrays in fixture files for better readability
- Updated code formatting throughout visualization and test files
Reviewed Changes
Copilot reviewed 60 out of 66 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/viz.cpp | Code formatting improvements including include reordering and bracket style updates |
| test/test.cpp | Complete migration from TAP framework to Google Test with parametrized tests |
| test/tap.hpp | Removed custom TAP testing framework header |
| test/tap.cpp | Removed custom TAP testing framework implementation |
| test/fixtures/*.cpp | Reformatted coordinate arrays and constructor parameters for better readability |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| #include <gtest/gtest.h> | ||
|
|
||
| #include <algorithm> | ||
| #include <iomanip> | ||
| #include <locale> | ||
| #include <sstream> | ||
|
|
||
| #include "fixtures/geometries.hpp" |
There was a problem hiding this comment.
[nitpick] Consider grouping system/standard library includes before project-specific includes for better organization and to follow common C++ conventions.
| INSTANTIATE_TEST_SUITE_P(FixtureTests, | ||
| EarcutAreaTest, | ||
| ::testing::ValuesIn(mapbox::fixtures::FixtureTester::collection()), | ||
| [](const ::testing::TestParamInfo<mapbox::fixtures::FixtureTester*>& info) { | ||
| std::string name = info.param->name; | ||
| std::replace(name.begin(), name.end(), '-', '_'); | ||
| std::replace(name.begin(), name.end(), '.', '_'); | ||
| return name; | ||
| }); |
There was a problem hiding this comment.
[nitpick] The lambda function for test name transformation should be extracted to a separate function to improve readability and potentially allow reuse if similar test instantiations are needed elsewhere.
| INSTANTIATE_TEST_SUITE_P(FixtureTests, | |
| EarcutAreaTest, | |
| ::testing::ValuesIn(mapbox::fixtures::FixtureTester::collection()), | |
| [](const ::testing::TestParamInfo<mapbox::fixtures::FixtureTester*>& info) { | |
| std::string name = info.param->name; | |
| std::replace(name.begin(), name.end(), '-', '_'); | |
| std::replace(name.begin(), name.end(), '.', '_'); | |
| return name; | |
| }); | |
| // Extracted function for test name transformation | |
| std::string TransformTestName(const ::testing::TestParamInfo<mapbox::fixtures::FixtureTester*>& info) { | |
| std::string name = info.param->name; | |
| std::replace(name.begin(), name.end(), '-', '_'); | |
| std::replace(name.begin(), name.end(), '.', '_'); | |
| return name; | |
| } | |
| INSTANTIATE_TEST_SUITE_P(FixtureTests, | |
| EarcutAreaTest, | |
| ::testing::ValuesIn(mapbox::fixtures::FixtureTester::collection()), | |
| TransformTestName); |
mourner
left a comment
There was a problem hiding this comment.
Very nice! Maybe let's disable code formatting on the fixtures though, and revert the changes? A bit too much whitespace on those after...
I considered that, but decided to bite the bullet and add it. I think it's much easier for a human to understand the nature of the polygons with those fixes. |
No description provided.