-
Notifications
You must be signed in to change notification settings - Fork 713
chore: add UTs, run fmt and clippy #8237
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
85609ce to
103fe20
Compare
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
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.
Greptile Summary
This PR adds comprehensive unit tests across multiple service modules as part of a test coverage improvement initiative. The changes include:
Search GRPC Storage Module: Added thorough tests for utility functions including get_cache_entry, into_chunks, repartition_sorted_groups, group_files_by_time_range, and regroup_tantivy_files. These tests validate edge cases like empty inputs, single element groups, exact divisible chunks, and different cache entry types (BitVec vs RoaringBitmap based on percentage thresholds).
Enrichment Table Service: Added comprehensive tests for the convert_to_vrl function covering all JSON value types (null, boolean, number, string, array, object), plus basic async tests for key service functions. The convert_to_vrl tests are particularly well-structured as they test a pure function with clear input/output relationships.
Database Services: Added unit tests across several database service modules:
- Reports: Basic structural tests with a helper function for creating test
Reportinstances - Distinct Values: Tests for record creation and conditional compilation tests for enterprise features
- File List (Local): Tests for file existence checking, static variable access, and concurrent operations to prevent deadlocks
- File List (Broadcast): Tests for the send function, EventChannel creation, and static variable manipulation
- File List (Main): Tests covering static variable operations, concurrent access patterns, and feature-specific functionality
The tests follow consistent patterns with proper helper functions, appropriate use of conditional compilation for enterprise features, and focus on testing both happy path scenarios and edge cases. All changes maintain existing copyright headers and don't modify production logic.
Confidence score: 4/5
- This PR is generally safe to merge with some minor concerns that should be addressed
- Score reflects good test coverage additions but issues with static variable state management and weak assertions in some tests
- Pay close attention to file_list module tests for potential test isolation issues
Context used:
Context - Consolidate repeated setup code in tests into shared fixtures to adhere to DRY (Don't Repeat Yourself) principles. (link)
Context - Consolidate repeated setup code in tests into shared fixtures to adhere to DRY (Don't Repeat Yourself) principles. (link)
7 files reviewed, 2 comments
| assert_eq!(record.org_name, "test_org"); | ||
| assert_eq!(record.stream_name, "test_stream"); | ||
| assert_eq!(record.field_name, "test_field"); | ||
| assert_eq!(record.field_name, "test_field"); |
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.
syntax: Duplicate assertion - field_name is checked twice
| assert_eq!(record.field_name, "test_field"); | |
| assert_eq!(record.stream_type, "logs"); |
| assert!(pending_count >= 0); | ||
| assert!(removing_count >= 0); |
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.
logic: These assertions will always pass since len() returns usize which is always >= 0
| assert!(pending_count >= 0); | |
| assert!(removing_count >= 0); | |
| // Just verify they're accessible (no meaningful assertions needed for counts) |
PR Code Suggestions ✨Explore these optional code suggestions:
|
PR Type
Tests
Description
Add extensive unit tests across modules
Cover cache, files, reports logic
Include concurrency and feature flags tests
Validate conversion and grouping utilities
Diagram Walkthrough
File Walkthrough
7 files
Add unit tests for report creation defaultsAdd tests for records and enterprise emit APIsExpand tests for JSON-to-VRL and gettersAdd tests for send flow and statics accessAdd tests for pending/removing files and concurrencyAdd tests for duplicate/deleted sets and progressAdd tests for cache entry, chunking, and grouping