-
Notifications
You must be signed in to change notification settings - Fork 148
Re-enable CI on Github Actions #128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
Copilot
AI
Sep 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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; | ||
| }); |
Copilot
AI
Sep 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.