Skip to content

Test : aggregation edge cases for list and scalar values#14170

Open
Achieve3318 wants to merge 2 commits intoinfiniflow:mainfrom
Achieve3318:test/memory-aggregation-edge-cases
Open

Test : aggregation edge cases for list and scalar values#14170
Achieve3318 wants to merge 2 commits intoinfiniflow:mainfrom
Achieve3318:test/memory-aggregation-edge-cases

Conversation

@Achieve3318
Copy link
Copy Markdown
Contributor

@Achieve3318 Achieve3318 commented Apr 16, 2026

This PR adds focused unit tests for aggregate_by_field in OceanBase memory utilities to improve behavior coverage for real-world input shapes.

  • Adds test coverage for list-valued aggregation fields, including whitespace trimming and skipping invalid list entries.
  • Adds test coverage for scalar field values to ensure blank/non-string values are ignored.
  • Confirms aggregation output remains correct and stable for mixed-quality message payloads.

Why this helps

It strengthens regression protection for aggregation logic used by memory retrieval flows, with no production code changes and minimal review risk.

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 16, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 16, 2026

Warning

Rate limit exceeded

@Achieve3318 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 44 seconds before requesting another review.

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 8 minutes and 44 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a3e4cd42-018c-4b0b-afa6-90543d64ad8b

📥 Commits

Reviewing files that changed from the base of the PR and between b7b8021 and e64e6a7.

📒 Files selected for processing (1)
  • test/unit_test/memory/utils/test_ob_conn_aggregation.py
📝 Walkthrough

Walkthrough

Added two new unit tests for the aggregate_by_field function to verify handling of list values with element aggregation and trimming, plus scalar value filtering of non-string and blank elements. No production code changes.

Changes

Cohort / File(s) Summary
Test Coverage Expansion
test/unit_test/memory/utils/test_ob_conn_aggregation.py
Added two new test methods to TestAggregateByField: one validates aggregation of list elements with whitespace trimming and blank-string filtering while handling non-string values gracefully; another validates scalar value handling by filtering non-strings and blank/whitespace-only strings.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 With whiskers twitched and tests so true,
Two new cases see us through,
Lists and scalars, trimmed with care,
Blanks ignored, the strings laid bare! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding tests for aggregation edge cases covering list and scalar values.
Description check ✅ Passed The description covers the PR purpose, test coverage scope, and benefits, though it lacks the structured template sections (problem statement, change type).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot dosubot Bot added 🐖api The modified files are located under directory 'api/apps/sdk' 🧪 test Pull requests that update test cases. labels Apr 16, 2026
Add focused unit tests to verify whitespace trimming and non-string filtering behavior in memory aggregation results.
@Achieve3318 Achieve3318 force-pushed the test/memory-aggregation-edge-cases branch from 7fa1917 to b7b8021 Compare April 16, 2026 14:11
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 16, 2026
@Achieve3318
Copy link
Copy Markdown
Contributor Author

Hi, @yingfeng , Could you review my PR, please?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/unit_test/memory/utils/test_ob_conn_aggregation.py`:
- Around line 57-74: The two new tests
test_aggregates_list_values_and_trims_whitespace and
test_ignores_non_string_and_blank_scalar_values are missing the required pytest
priority markers; add the appropriate pytest marker (e.g., `@pytest.mark.p1` or
`@pytest.mark.p2/`@pytest.mark.p3 per project guidelines) above each test function
so they include a priority marker, and ensure you import pytest if not already
present so the decorators resolve.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 16f5e3ca-fd10-4cdf-b683-109f1fb55000

📥 Commits

Reviewing files that changed from the base of the PR and between f906a20 and b7b8021.

📒 Files selected for processing (1)
  • test/unit_test/memory/utils/test_ob_conn_aggregation.py

Comment thread test/unit_test/memory/utils/test_ob_conn_aggregation.py
Ensure new unit tests follow test suite marker conventions for CI and reviewer checks.
@Achieve3318
Copy link
Copy Markdown
Contributor Author

Hi, @yingfeng , Could you review my PR, please?

@Achieve3318
Copy link
Copy Markdown
Contributor Author

hi, @yingfeng , Could you review my PR?

@Achieve3318
Copy link
Copy Markdown
Contributor Author

Hi, @yingfeng , Could you review my PR, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐖api The modified files are located under directory 'api/apps/sdk' size:XS This PR changes 0-9 lines, ignoring generated files. 🧪 test Pull requests that update test cases.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant