test(serde): comprehensive test coverage for all backends#99
test(serde): comprehensive test coverage for all backends#9916bit-ykiko wants to merge 4 commits intomainfrom
Conversation
Extend torture tests across all 5 backends to maximize standard suite coverage, add missing tests for behavior attrs (with<>/as<>/literal<>), RawValue (JSON/Bincode), and serde_error<Kind> API. - TOML: add ATTRS, TAGGED_VARIANTS_NO_INTERNAL, VARIANT_TOML_SAFE - Bincode: add ATTRS_BINCODE_SAFE, BEHAVIOR_ATTRS - FlatBuffers: add VARIANT_WIRE_SAFE - All text backends: add BEHAVIOR_ATTRS (with<Adapter>, as<Target>, literal) - JSON: 17 RawValue tests (serialize/deserialize/roundtrip/struct) - Bincode: 5 RawValue tests (roundtrip/struct) - New serde_error unit tests: 41 tests for factory methods, path manipulation, location tracking, formatting, comparison, edge cases - Document omitted features per backend with rationale in comments 739 → 810 tests, all passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds extensive unit tests: RawValue roundtrips for bincode and simdjson, a new serde::error test suite, behavior-attribute test types/macros, and expanded torture test invocations across multiple backends to exercise attribute and variant cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/serde/standard_case_suite.h`:
- Around line 1217-1250: The current SERDE_STANDARD_TEST_CASES_BEHAVIOR_ATTRS
macro uses plain int fields (from make_behavior_with_payload and
make_behavior_as_payload) so dropping behavior::with / behavior::as would still
pass; to fix, change the test inputs created by make_behavior_with_payload,
make_behavior_as_payload, and make_behavior_literal_payload to include a field
type that only serializes/deserializes via the behavior adapter (i.e., not a raw
int), or add explicit assertions against the encoded representation in the
backend-specific suites; locate the macro and the factory functions
(make_behavior_with_payload, make_behavior_as_payload,
make_behavior_literal_payload) and either (a) replace
doubled_value/wide_value/kind test values with an adapter-only type instance and
assert annotated_value(output->...) equals annotated_value(input....) for that
type, or (b) add checks that the serialized bytes/JSON produced by the
serializer contain the adapter-specific encoding for those fields to ensure the
behavior hooks actually ran.
- Around line 489-530: Add unit tests that exercise the standalone adapter hooks
by invoking DoubleIntAdapter through the serializer/deserializer paths that call
Adapter::serialize and Adapter::deserialize (e.g., use
behavior::with<DoubleIntAdapter> or directly call the top-level serde
serialize/deserialize helpers so the code paths in
include/eventide/serde/serde/utils/field_dispatch.h that dispatch to
Adapter::serialize/Adapter::deserialize are exercised); specifically add tests
that call DoubleIntAdapter::serialize(...) and
DoubleIntAdapter::deserialize(...) (or run the serializer/deserializer in a way
that resolves to those overloads) to verify the multiply-by-2 on serialize and
divide-by-2 on deserialize behavior rather than only testing
serialize_field/deserialize_field.
- Around line 516-517: The header uses std::string_view in the serialize_field
function (SerializeStruct& s_struct, std::string_view name, const int& v) but
does not include the required <string_view> header; add an explicit `#include`
<string_view> at the top of this file so std::string_view is defined and
portable across compilers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcbd3e1e-3c90-4a85-87ca-c9643a42e6de
📒 Files selected for processing (9)
tests/unit/serde/bincode/bincode_tests.cpptests/unit/serde/bincode/bincode_torture_tests.cpptests/unit/serde/bincode/serde_error_tests.cpptests/unit/serde/content/yyjson_torture_tests.cpptests/unit/serde/flatbuffers/flatbuffers_torture_tests.cpptests/unit/serde/json/simdjson_tests.cpptests/unit/serde/json/simdjson_torture_tests.cpptests/unit/serde/standard_case_suite.htests/unit/serde/toml/toml_torture_tests.cpp
| /// Adapter for behavior::with<DoubleIntAdapter>. | ||
| /// On serialize: emits the int value multiplied by 2. | ||
| /// On deserialize: reads the raw int and divides by 2. | ||
| /// | ||
| /// Supports both the standalone path (Adapter::serialize / Adapter::deserialize) | ||
| /// and the struct-field path (Adapter::serialize_field / Adapter::deserialize_field). | ||
| struct DoubleIntAdapter { | ||
| // Standalone serialize: called as Adapter::serialize(s, v) from serde.h | ||
| template <typename S> | ||
| static auto serialize(S& s, const int& v) { | ||
| return s.serialize_int(static_cast<std::int64_t>(v) * 2); | ||
| } | ||
|
|
||
| // Standalone deserialize: called as Adapter::deserialize(d, v) from serde.h | ||
| template <typename D> | ||
| static auto deserialize(D& d, int& v) { | ||
| std::int64_t raw{}; | ||
| auto status = d.deserialize_int(raw); | ||
| if(!status) { | ||
| return status; | ||
| } | ||
| v = static_cast<int>(raw / 2); | ||
| return status; | ||
| } | ||
|
|
||
| // Struct-field serialize: called as Adapter::serialize_field(s_struct, name, v) | ||
| template <typename SerializeStruct> | ||
| static auto serialize_field(SerializeStruct& s_struct, std::string_view name, const int& v) { | ||
| return s_struct.serialize_field(name, static_cast<std::int64_t>(v) * 2); | ||
| } | ||
|
|
||
| // Struct-field deserialize: called as Adapter::deserialize_field(d_struct, v) | ||
| template <typename DeserializeStruct> | ||
| static auto deserialize_field(DeserializeStruct& d_struct, int& v) { | ||
| std::int64_t raw{}; | ||
| auto status = d_struct.deserialize_value(raw); | ||
| if(!status) { | ||
| return status; | ||
| } | ||
| v = static_cast<int>(raw / 2); | ||
| return status; | ||
| } |
There was a problem hiding this comment.
The standalone adapter hooks are still untested.
The new suite only exercises BehaviorWithPayload. For annotated struct fields, dispatch goes through Adapter::serialize_field / Adapter::deserialize_field in include/eventide/serde/serde/utils/field_dispatch.h:75-84 and include/eventide/serde/serde/utils/field_dispatch.h:183-192, so the standalone serialize / deserialize overloads added here never get covered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/serde/standard_case_suite.h` around lines 489 - 530, Add unit
tests that exercise the standalone adapter hooks by invoking DoubleIntAdapter
through the serializer/deserializer paths that call Adapter::serialize and
Adapter::deserialize (e.g., use behavior::with<DoubleIntAdapter> or directly
call the top-level serde serialize/deserialize helpers so the code paths in
include/eventide/serde/serde/utils/field_dispatch.h that dispatch to
Adapter::serialize/Adapter::deserialize are exercised); specifically add tests
that call DoubleIntAdapter::serialize(...) and
DoubleIntAdapter::deserialize(...) (or run the serializer/deserializer in a way
that resolves to those overloads) to verify the multiply-by-2 on serialize and
divide-by-2 on deserialize behavior rather than only testing
serialize_field/deserialize_field.
| static auto serialize_field(SerializeStruct& s_struct, std::string_view name, const int& v) { | ||
| return s_struct.serialize_field(name, static_cast<std::int64_t>(v) * 2); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++20, is std::string_view guaranteed to be declared by <string>, or should a header that uses std::string_view include <string_view> explicitly?
💡 Result:
In C++20, std::string_view is not guaranteed to be declared by including <string> alone. A header that uses std::string_view must include <string_view> explicitly. The C++ standard specifies std::basic_string_view (and typedef std::string_view) in the <string_view> header synopsis [string.view.synop], not in <string> [string.syn]. Standard library headers do not have guaranteed transitive includes unless explicitly stated in their synopses, and <string> synopsis does not include <string_view> or declare basic_string_view. cppreference.com confirms separate headers: <string> defines basic_string and char_traits, while <string_view> defines basic_string_view. std::basic_string has a converting operator to string_view (C++17, constexpr C++20), but this requires string_view to be declared/visible, necessitating <string_view>. Relying on implementation-specific transitive includes is non-portable; explicit inclusion ensures compliance across compilers (e.g., libc++ header removal policies).
Citations:
- 1: https://en.cppreference.com/w/cpp/header/string_view.html
- 2: https://en.cppreference.com/w/cpp/header/string_view
- 3: https://timsong-cpp.github.io/cppwp/string.view.synop
- 4: https://learn.microsoft.com/en-us/cpp/standard-library/string-view?view=msvc-170
- 5: https://en.cppreference.com/w/cpp/header/string.html
- 6: http://eel.is/c++draft/string.syn
- 7: https://www.cppreference.com/w/cpp/string/basic_string_view.html
- 8: https://en.cppreference.com/w/cpp/string/basic_string/operator_basic_string_view
- 9: https://stackoverflow.com/questions/26614983/which-headers-in-the-c-standard-library-are-guaranteed-to-include-another-head
- 10: https://eel.is/c++draft/using.headers
🏁 Script executed:
#!/bin/bash
# Check if the file includes <string_view>
echo "=== Checking for <string_view> include ==="
grep -n '#include.*<string_view>' tests/unit/serde/standard_case_suite.h || echo "No <string_view> include found"
echo -e "\n=== Line 516 content ==="
sed -n '516p' tests/unit/serde/standard_case_suite.h
echo -e "\n=== Context around line 516 (lines 514-520) ==="
sed -n '514,520p' tests/unit/serde/standard_case_suite.h
echo -e "\n=== All includes in the file ==="
head -50 tests/unit/serde/standard_case_suite.h | grep '#include'Repository: clice-io/eventide
Length of output: 1172
Include <string_view> explicitly.
Line 516 uses std::string_view, but this header never includes <string_view>. The C++ standard specifies std::string_view in the <string_view> header, not in <string>, so explicit inclusion is required for portability and correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/serde/standard_case_suite.h` around lines 516 - 517, The header
uses std::string_view in the serialize_field function (SerializeStruct&
s_struct, std::string_view name, const int& v) but does not include the required
<string_view> header; add an explicit `#include` <string_view> at the top of this
file so std::string_view is defined and portable across compilers.
| #define SERDE_STANDARD_TEST_CASES_BEHAVIOR_ATTRS(rt) \ | ||
| TEST_CASE(standard_behavior_attrs_roundtrip) { \ | ||
| { \ | ||
| auto input = eventide::serde::standard_case::make_behavior_with_payload(); \ | ||
| auto output = (rt)(input); \ | ||
| ASSERT_TRUE(output.has_value()); \ | ||
| EXPECT_EQ(output->id, input.id); \ | ||
| EXPECT_EQ(eventide::serde::annotated_value(output->doubled_value), \ | ||
| eventide::serde::annotated_value(input.doubled_value)); \ | ||
| } \ | ||
| { \ | ||
| auto input = eventide::serde::standard_case::make_behavior_with_payload(); \ | ||
| input.doubled_value = 0; \ | ||
| auto output = (rt)(input); \ | ||
| ASSERT_TRUE(output.has_value()); \ | ||
| EXPECT_EQ(eventide::serde::annotated_value(output->doubled_value), 0); \ | ||
| } \ | ||
| { \ | ||
| auto input = eventide::serde::standard_case::make_behavior_as_payload(); \ | ||
| auto output = (rt)(input); \ | ||
| ASSERT_TRUE(output.has_value()); \ | ||
| EXPECT_EQ(output->id, input.id); \ | ||
| EXPECT_EQ(eventide::serde::annotated_value(output->wide_value), \ | ||
| eventide::serde::annotated_value(input.wide_value)); \ | ||
| } \ | ||
| { \ | ||
| auto input = eventide::serde::standard_case::make_behavior_literal_payload(); \ | ||
| auto output = (rt)(input); \ | ||
| ASSERT_TRUE(output.has_value()); \ | ||
| EXPECT_EQ(output->id, input.id); \ | ||
| EXPECT_EQ(eventide::serde::annotated_value(output->kind), \ | ||
| eventide::serde::annotated_value(input.kind)); \ | ||
| } \ | ||
| } |
There was a problem hiding this comment.
These assertions don't prove the behavior annotations ran.
BehaviorWithPayload and BehaviorAsPayload only roundtrip plain int fields, so a regression that drops behavior::with / behavior::as on both serialize and deserialize would still pass every check here. To really pin those hooks, use a field type that only works via the adapter/target conversion, or assert on the encoded form in the backend-specific suites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/serde/standard_case_suite.h` around lines 1217 - 1250, The current
SERDE_STANDARD_TEST_CASES_BEHAVIOR_ATTRS macro uses plain int fields (from
make_behavior_with_payload and make_behavior_as_payload) so dropping
behavior::with / behavior::as would still pass; to fix, change the test inputs
created by make_behavior_with_payload, make_behavior_as_payload, and
make_behavior_literal_payload to include a field type that only
serializes/deserializes via the behavior adapter (i.e., not a raw int), or add
explicit assertions against the encoded representation in the backend-specific
suites; locate the macro and the factory functions (make_behavior_with_payload,
make_behavior_as_payload, make_behavior_literal_payload) and either (a) replace
doubled_value/wide_value/kind test values with an adapter-only type instance and
assert annotated_value(output->...) equals annotated_value(input....) for that
type, or (b) add checks that the serialized bytes/JSON produced by the
serializer contain the adapter-specific encoding for those fields to ensure the
behavior hooks actually ran.
Add 27 test cases for previously untested attribute scenarios: - skip_if_empty: serialize omission, deser missing-field behavior - skip_if_default: serialize omission with default_value, deser fallback - literal<>: document metadata-only behavior (no wire rename/validation) - Multiple aliases (3 names): deser from primary, alt1, alt2 - Multiple flatten fields: two sub-structs flattened into parent - deny_unknown_fields + nested struct: strict at parent, lenient inside - rename_all: upper_camel, upper_snake, lower_snake policies - enum_string: all 4 policies (lower_camel, upper_camel, lower_snake, upper_snake) with multi-word enum values 810 → 837 tests, all passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 27 attribute test cases for skip_if_empty, skip_if_default, literal metadata behavior, multiple aliases, multiple flatten, deny_unknown_fields with nested structs, all rename_all policies, and all enum_string policies. Split toml_torture_tests.cpp into two files (basic types + attrs/variants) to fix MSVC C1128 section limit exceeded error on Windows CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/serde/json/simdjson_config_tests.cpp (1)
60-67: Consider removing currently unused enum config structs.
enum_string_config_upper_snakeandenum_string_config_lower_camelare declared but not referenced in this file. If they are not planned for immediate use, trimming them would reduce local test noise.✂️ Optional cleanup diff
-struct enum_string_config_upper_snake { - using enum_rename = rename_policy::upper_snake; -}; - -struct enum_string_config_lower_camel { - using enum_rename = rename_policy::lower_camel; -};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/serde/json/simdjson_config_tests.cpp` around lines 60 - 67, The two unused test-only structs enum_string_config_upper_snake and enum_string_config_lower_camel are declared but never referenced; either remove these declarations from tests/unit/serde/json/simdjson_config_tests.cpp or reference them where intended (e.g., in a test fixture or test case that verifies rename_policy behavior). Locate the struct definitions (enum_string_config_upper_snake, enum_string_config_lower_camel) and delete them if not needed, or wire them into the relevant test functions that assert enum renaming (so the symbols are actually used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/serde/json/simdjson_config_tests.cpp`:
- Around line 60-67: The two unused test-only structs
enum_string_config_upper_snake and enum_string_config_lower_camel are declared
but never referenced; either remove these declarations from
tests/unit/serde/json/simdjson_config_tests.cpp or reference them where intended
(e.g., in a test fixture or test case that verifies rename_policy behavior).
Locate the struct definitions (enum_string_config_upper_snake,
enum_string_config_lower_camel) and delete them if not needed, or wire them into
the relevant test functions that assert enum renaming (so the symbols are
actually used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2eefe8c4-f50a-4e55-b710-15c1ef8378ac
📒 Files selected for processing (2)
tests/unit/serde/json/simdjson_attrs_tests.cpptests/unit/serde/json/simdjson_config_tests.cpp
- C4834: add (void) casts for [[nodiscard]] mark_invalid() calls in constructors where the return value is intentionally discarded (json/deserializer.h: 5 sites, content/deserializer.h: 4 sites) - C5232: rewrite `*a != *b` as `!(*a == *b)` in TreeNode::operator== to avoid MSVC C++20 recursive rewritten-candidate warning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
with<Adapter>,as<Target>,literal<>behavior attribute tests tostandard_case_suite.hand enable in 4 backendsserde_error<Kind>API unit tests (41 tests covering factory methods, path manipulation, location, formatting, comparison)739 → 810 tests, all passing.
Backend coverage matrix after this PR
Test plan
pixi run formatapplied🤖 Generated with Claude Code
Summary by CodeRabbit