Rework complex CU tests to update schema a bit more generically #766
Conversation
WalkthroughThe test suite was refactored to use a new "LargeEvent" Avro schema and corresponding payload generator, replacing the previous "BeaconTop" schema. All references, field names, data generators, and tests were updated to align with the new schema structure, field names, and event data. Changes
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
piyush-zlai
requested review from
chewy-zlai,
david-zlai,
sean-zlai,
tchow-zlai and
varant-zlai
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
online/src/test/scala/ai/chronon/online/test/CatalystUtilComplexAvroTest.scala (1)
186-189: Schema definition could be improved.Consider moving the large schema string to a separate file or splitting it for better readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
online/src/test/scala/ai/chronon/online/test/CatalystUtilComplexAvroTest.scala(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: service_commons_tests
- GitHub Check: service_tests
- GitHub Check: flink_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: service_tests
- GitHub Check: api_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: online_tests
- GitHub Check: api_tests
- GitHub Check: flink_tests
- GitHub Check: online_tests
- GitHub Check: aggregator_tests
- GitHub Check: aggregator_tests
- GitHub Check: scala_compile_fmt_fix
🔇 Additional comments (9)
online/src/test/scala/ai/chronon/online/test/CatalystUtilComplexAvroTest.scala (9)
21-24: Schema renaming looks good.The class comment accurately describes test purpose and import updated correctly.
26-33: Field name changes in select expressions are consistent.All references properly updated from previous schema field names to new LargeEvent fields.
35-41: Where conditions updated correctly.Field paths changed from "properties_top" to "attributes" consistently.
46-52: Schema processing references updated properly.Consistent schema reference usage throughout the processing logic.
66-67: Good update to result validation.Item ID validation logic correctly updated.
73-85: Test case for add_cart event updated correctly.All field references and method calls properly adjusted to use the new schema.
194-247: Base event creation looks good.All fields properly renamed and structured according to the new schema.
249-266: Default attributes creation updated correctly.Proper field setup for the nested attributes record.
268-284: Modified event creation properly updated.The function maintains the same behavior while adapting to the new schema structure.
| it should "match event condition (favorite) with listing IDs" in { | ||
| val favoriteEvent = createModifiedEvent( | ||
| Map( | ||
| "event_name" -> "backend_favorite_item2", | ||
| "event_logger" -> "native", | ||
| "event_source" -> "ios", | ||
| "properties_top.region" -> "JP", | ||
| "properties_top.listing_id" -> "789012,456789" | ||
| "event" -> "favorite", | ||
| "logger" -> "native", | ||
| "src" -> "ios", | ||
| "attributes.region" -> "JP", | ||
| "attributes.item_id" -> "789012,456789" | ||
| )) | ||
| val payloadBase64 = serializeToBase64(favoriteEvent) | ||
| val result = processEvent(payloadBase64) | ||
| // expect 2 rows for each of the listings, we check those | ||
| assert(result.size == 2) | ||
| assert(result.map(r => r("listing_id")).map(_.toString).toSet == Set("456789", "789012")) | ||
| assert(result.map(r => r("item_id")).map(_.toString).toSet == Set("456789", "789012")) | ||
| assert(result.map(r => r("favorite")).toSet == Set(1)) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistent type handling in assertions.
Line 127 converts to string before comparison while line 66 doesn't need conversion.
🏁 Script executed:
#!/bin/bash
# Check if the code is inconsistent in handling item_id types
grep -r "item_id" --include="*.scala" ./online/src/test/scala/Length of output: 3460
Unify item_id assertion types in tests
The favorite-event test casts to String, while other tests compare Longs. Prefer comparing Longs for consistency:
• File: online/src/test/scala/ai/chronon/online/test/CatalystUtilComplexAvroTest.scala
Lines 127–128
- assert(result.map(r => r("item_id")).map(_.toString).toSet == Set("456789", "789012"))
+ assert(result.map(r => r("item_id")).toSet == Set(456789L, 789012L))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it should "match event condition (favorite) with listing IDs" in { | |
| val favoriteEvent = createModifiedEvent( | |
| Map( | |
| "event_name" -> "backend_favorite_item2", | |
| "event_logger" -> "native", | |
| "event_source" -> "ios", | |
| "properties_top.region" -> "JP", | |
| "properties_top.listing_id" -> "789012,456789" | |
| "event" -> "favorite", | |
| "logger" -> "native", | |
| "src" -> "ios", | |
| "attributes.region" -> "JP", | |
| "attributes.item_id" -> "789012,456789" | |
| )) | |
| val payloadBase64 = serializeToBase64(favoriteEvent) | |
| val result = processEvent(payloadBase64) | |
| // expect 2 rows for each of the listings, we check those | |
| assert(result.size == 2) | |
| assert(result.map(r => r("listing_id")).map(_.toString).toSet == Set("456789", "789012")) | |
| assert(result.map(r => r("item_id")).map(_.toString).toSet == Set("456789", "789012")) | |
| assert(result.map(r => r("favorite")).toSet == Set(1)) | |
| } | |
| it should "match event condition (favorite) with listing IDs" in { | |
| val favoriteEvent = createModifiedEvent( | |
| Map( | |
| "event" -> "favorite", | |
| "logger" -> "native", | |
| "src" -> "ios", | |
| "attributes.region" -> "JP", | |
| "attributes.item_id" -> "789012,456789" | |
| )) | |
| val payloadBase64 = serializeToBase64(favoriteEvent) | |
| val result = processEvent(payloadBase64) | |
| // expect 2 rows for each of the listings, we check those | |
| assert(result.size == 2) | |
| assert(result.map(r => r("item_id")).toSet == Set(456789L, 789012L)) | |
| assert(result.map(r => r("favorite")).toSet == Set(1)) | |
| } |
## Summary ## Checklist - [ ] Added Unit Tests - [X] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated test cases to use a new event schema with revised field names and structure. - Renamed and adjusted test data and helper methods to align with the new schema and naming conventions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary ## Checklist - [ ] Added Unit Tests - [X] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated test cases to use a new event schema with revised field names and structure. - Renamed and adjusted test data and helper methods to align with the new schema and naming conventions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary ## Cheour clientslist - [ ] Added Unit Tests - [X] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated test cases to use a new event schema with revised field names and structure. - Renamed and adjusted test data and helper methods to align with the new schema and naming conventions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Checklist
Summary by CodeRabbit