feat(integration-tests): Add package search testing verification infra.#2235
feat(integration-tests): Add package search testing verification infra.#2235quinntaylormitchell wants to merge 31 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a typed SearchArgs builder, ClpPackageSearchType modes, functions to construct and run CLP package search actions, grep-based verification helpers that normalize results per mode, and a new dataset metadata field plus validation for a single-match file. ChangesSearch Test Utilities
Dataset Metadata
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Builder as SearchArgs / search_clp_package
participant CLP as CLP subprocess
participant Verifier as verify_search_action
participant Grep as grep subprocess
Test->>Builder: request ClpAction(dataset, type, query)
Builder-->>Test: ClpAction (cmd list, SearchArgs)
Test->>CLP: run ClpAction subprocess
CLP-->>Test: exit code + stdout
Test->>Verifier: verify_search_action(action, type, dataset)
Verifier->>Verifier: check exit code & args
Verifier->>Grep: run mode-specific grep on dataset logs or file path
Grep-->>Verifier: grep stdout / exit code
Verifier->>Verifier: normalize outputs and compare
Verifier-->>Test: VerificationResult.ok() or fail(message)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
quinntaylormitchell
changed the title
![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 `@integration-tests/tests/package_tests/utils/search.py`:
- Line 134: When running in FILE_PATH mode, guard against empty metadata
file_names before indexing: check that dataset.metadata.file_names is non-empty
(e.g., len(dataset.metadata.file_names) > 0 or truthiness) before using
dataset.metadata.file_names[0]; if empty, raise a clear assertion or ValueError
with a useful message rather than letting IndexError bubble up. Update the
assignment to args.file_path to only occur after this guard; reference the
existing identifiers args.file_path, dataset.metadata.file_names, and FILE_PATH
mode to locate and modify the code.
- Around line 176-177: Replace the bare assert on "args = action.args" with
explicit error handling: check "isinstance(args, SearchArgs)" and if false
return a VerificationResult.fail(...) with a clear message (matching other
validations in this function). Use the same VerificationResult type used
elsewhere in this file so invalid action.args is handled consistently rather
than being stripped by Python -O; reference the variables "args" and
"action.args" and the classes "SearchArgs" and "VerificationResult".
- Around line 184-188: The check treating any non-zero grep exit code as failure
is too strict: GNU grep returns 1 when no matches are found. Update the
condition in the block that inspects grep_action.completed_proc.returncode to
only fail when the return code is neither 0 nor 1 (i.e., allow 0 and 1), and
keep the pytest.fail call that references grep_action.log_file_path for true
errors; ensure you still reference grep_action.completed_proc.returncode and
pytest.fail in the same location.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ef0fc03e-e41f-4ba2-b226-b05ab2ce3723
📒 Files selected for processing (2)
integration-tests/tests/package_tests/utils/search.pyintegration-tests/tests/utils/classes.py
There was a problem hiding this comment.
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 `@integration-tests/tests/package_tests/utils/search.py`:
- Around line 282-286: The COUNT_BY_TIME branch currently uses re.search which
only returns the first "count: <n>" match; change the logic in the case handling
for ClpPackageSearchType.COUNT_RESULTS | ClpPackageSearchType.COUNT_BY_TIME to
use re.findall(r"count: (\d+)", search_result) (or an iterator like re.finditer)
to collect all count buckets, return them joined with "\n" and a trailing
newline, and call pytest.fail if the resulting list is empty; keep the same
match formatting and error message behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 22395b96-f587-4100-b7c2-fa030589ad7f
📒 Files selected for processing (1)
integration-tests/tests/package_tests/utils/search.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
integration-tests/tests/package_tests/utils/search.py (2)
175-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
assertwith explicit verification failure handling.
assertcan be stripped with Python optimization flags, which can bypass this validation path. Use explicit runtime handling and return a typed verification failure instead.Suggested fix
args = action.args - assert isinstance(args, SearchArgs) + if not isinstance(args, SearchArgs): + return action.fail_verification( + "Search verification requires `action.args` to be a SearchArgs instance." + )In Python, are assert statements removed when running with optimization flags like -O?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/tests/package_tests/utils/search.py` around lines 175 - 176, The code uses an assert to check the type of action.args which can be bypassed with Python -O; replace the assert with an explicit runtime check on action.args (e.g., inspect isinstance(action.args, SearchArgs)) and on failure raise or return the appropriate verification failure value used by this test harness (do not rely on AssertionError). Update the handling around args/action.args and SearchArgs so the function returns or raises the typed verification failure the test framework expects instead of using assert.
269-273:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCOUNT_BY_TIME parsing only reads the first bucket.
re.searchonly captures onecount:value, so multi-bucket count-by-time output is verified incorrectly. Aggregate all buckets before comparing.Suggested fix
case ClpPackageSearchType.COUNT_RESULTS | ClpPackageSearchType.COUNT_BY_TIME: - match = re.search(r"count: (\d+)", search_result) - if match: - return match.group(1) + "\n" + counts = [int(value) for value in re.findall(r"count:\s*(\d+)", search_result)] + if counts: + return f"{sum(counts)}\n" pytest.fail(f"The search result '{search_result}' wasn't in the correct format.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/tests/package_tests/utils/search.py` around lines 269 - 273, The COUNT_BY_TIME branch currently uses re.search which only captures the first "count: X" and misses additional buckets; update the block handling ClpPackageSearchType.COUNT_RESULTS | ClpPackageSearchType.COUNT_BY_TIME to use re.findall(r"count: (\d+)", search_result), convert all matches to integers, sum them (or otherwise aggregate all bucket counts as required), and return the aggregated value as a string with a trailing newline (e.g., str(total) + "\n"); keep the pytest.fail call if no matches are found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@integration-tests/tests/package_tests/utils/search.py`:
- Around line 175-176: The code uses an assert to check the type of action.args
which can be bypassed with Python -O; replace the assert with an explicit
runtime check on action.args (e.g., inspect isinstance(action.args, SearchArgs))
and on failure raise or return the appropriate verification failure value used
by this test harness (do not rely on AssertionError). Update the handling around
args/action.args and SearchArgs so the function returns or raises the typed
verification failure the test framework expects instead of using assert.
- Around line 269-273: The COUNT_BY_TIME branch currently uses re.search which
only captures the first "count: X" and misses additional buckets; update the
block handling ClpPackageSearchType.COUNT_RESULTS |
ClpPackageSearchType.COUNT_BY_TIME to use re.findall(r"count: (\d+)",
search_result), convert all matches to integers, sum them (or otherwise
aggregate all bucket counts as required), and return the aggregated value as a
string with a trailing newline (e.g., str(total) + "\n"); keep the pytest.fail
call if no matches are found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07eaf020-0cb0-448f-80b7-6cc6a26a3283
📒 Files selected for processing (2)
integration-tests/tests/package_tests/utils/search.pyintegration-tests/tests/utils/classes.py
Bill-hbrhbr
left a comment
Bill-hbrhbr
left a comment
There was a problem hiding this comment.
This PR doesn't actually include any search tests right? Let's include them instead of simply introducing a portion of the search test infra.
Also, most of my concerns are elaborated in the comments.
quinntaylormitchell
changed the title
clp-json search verification infra.
quinntaylormitchell
changed the title
clp-json search verification infra.
quinntaylormitchell
changed the title
| class EpochMsTimestampFormat(BaseModel): | ||
| """Indicates a timestamp encoded as an integer number of milliseconds since the UNIX epoch.""" | ||
|
|
||
| kind: Literal["epoch_ms"] |
There was a problem hiding this comment.
| kind: Literal["epoch_ms"] | |
| type: Literal["epoch"] | |
| unit: Literal["s", "ms", "us", "ns"] |
I like the type key name better than the kind. Sounds more canonical.
Also I think it be beneficial to have epoch as a category and ms in a separate unit key.
There was a problem hiding this comment.
I like the
typekey name better than thekind.
Agreed; changed to type
I think it be beneficial to have epoch as a category and ms in a separate unit key
I think I’m going to leave it as epoch_ms for now, but later if/when we add datasets with other units, I’m happy to update it.
| @@ -0,0 +1,350 @@ | |||
| """Search verification helpers specific to the clp-json package.""" | |||
There was a problem hiding this comment.
since there are no actual new additions of clp-json tests, can we defer this to the PR where we add the actual tests?
There was a problem hiding this comment.
I'm not sure why the tests from test_clp_json.py were removed, since that file seems to contain a fairly comprehensive set of utilities for validating clp-json query results. My two comments:
- The JSON parsing utilities would probably be better placed in the general utilities folder so they can be reused by other tests.
- As a starting point, I think we could simply verify the total match count when testing clp-json search results. That would let us add basic coverage without having to deal with parsing JSON strings right away.
There was a problem hiding this comment.
can we defer this to the PR where we add the actual tests
Will discuss offline
I'm not sure why the tests from test_clp_json.py were removed
Not sure what you are referring to; other than the removal of the placeholder test (which I have now restored) this PR doesn't remove any utilities for validating clp-json query results; it adds them
The JSON parsing utilities would probably be better placed in the general utilities folder so they can be reused by other tests.
There are a lot of specialized methods that use (or are related to) JSON in this module, but other than moving strip_surrounding_quotes to the general utilities folder (which I have done) I don't see a lot of opportunity for reuse here. We can move any method(s) we choose at a later date if there ends up being concrete opportunity for reuse later on.
I think we could simply verify the total match count when testing clp-json search results.
Will discuss offline
| logger.info("Verifying the compression of the 'text_multifile' dataset.") | ||
| result = verify_compress_unstructured_clp_json(action, clp_package, text_multifile) | ||
| assert result, result.failure_message | ||
|
|
There was a problem hiding this comment.
ditto. let's not touch this part until we decide to include clp-json search tests.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| DEFAULT_COUNT_BY_TIME_INTERVAL = 10 |
There was a problem hiding this comment.
this is currently not referenced anywhere. probably belonged to a removed clp-json search test.
There was a problem hiding this comment.
Right on, thank you
| DEFAULT_COUNT_BY_TIME_INTERVAL = 10 | ||
|
|
||
|
|
||
| def parse_timestamp_to_epoch_ms(timestamp: str, strptime_pattern: str) -> int: |
There was a problem hiding this comment.
this should ideally be a function of class StrptimeTimestampFormat(BaseModel):
can be renamed to something like to_epoch_ms(timestamp: str).
There was a problem hiding this comment.
I’ve combined the two classes into a single class TimestampFormat and added a method called to_epoch_ms that handles the format conversion. If that’s not aligned with what you meant, let me know.
| return [self.test_data_dir] | ||
|
|
||
|
|
||
| class EpochMsTimestampFormat(BaseModel): |
There was a problem hiding this comment.
In general, I think these new classes would be better placed in a dedicated timestamps.py module under the same utils directory. The current classes.py seems to be accumulating too many unrelated classes, so separating the timestamp-related ones into their own module would keep things better organized.
There was a problem hiding this comment.
Sure, I’ve added it to a new file integration-tests/tests/utils/timestamps.py.
| return filtered_entries | ||
|
|
||
|
|
||
| def _resolve_timestamp_to_ms( |
There was a problem hiding this comment.
This can be an API that's provided respectively by EpochMsTimestampFormat and StrptimeTimestampFormat. You can check if the format string is convertible for strptime and whether the value is integer for epoch timestamp.
There was a problem hiding this comment.
If CLP is strictly taking epoch ms timestamps, EpochMsTimestampFormat and StrptimeTimestampFormat can share a base TimestampFormat class that provides the to_epoch_ms virtual method. Then TimestampFormat can represent more than a mere type alias.
There was a problem hiding this comment.
I’ve combined the two classes into a single class TimestampFormat and added a method called to_epoch_ms that handles the format conversion. If that’s not aligned with what you meant, let me know.
| """ | ||
| Constructs a key-value pair from a query string. The query is split on the first | ||
| `KV_DELIMITER_COLON` that is not inside a quoted group (`"..."` or `'...'`). | ||
|
|
||
| :param query: | ||
| :return: `(KEY, VALUE)` if `KV_DELIMITER_COLON` is present, else | ||
| `(WILDCARD_MULTIMATCH_CHAR, VALUE)`. | ||
| """ |
There was a problem hiding this comment.
using a constant KV_DELIMITER_COLON inside the docstring makes it a bit harder to read.
There was a problem hiding this comment.
Trying to align with the fact that KV_DELIMITER_COLON is what we use in the function; see comment below about changing to KV_DELIMITER; that might make it more readable.
| elif char in ('"', "'"): | ||
| open_quote = char | ||
| elif char == KV_DELIMITER_COLON: |
There was a problem hiding this comment.
Not sure why we use the KV_DELIMITER_COLON constant in one if statement but raw quote characters in another. For simple characters that are unlikely to change, I'd just use the literal characters consistently in both the code and the docstrings.
BTW, that's already the approach taken for the quote characters in _strip_surrounding_quotes's docstring.
There was a problem hiding this comment.
The main reason is that a delimiter can be anything (hypothetically), but the term "quotation marks" refers to a specific set of characters (either " or '). To me, it seems like delimiter characters are much more likely to change than quotation marks.
That being said, I'll rename KV_DELIMITER_COLON to KV_DELIMITER to fully recognize that it may change.
Also: not sure what you mean re. the docstrings; fairly sure that all methods in this file refer to quotation marks as literals and to the delimiter character as KV_DELIMITER_COLON. Did I misunderstand you?
| @@ -0,0 +1,179 @@ | |||
| """Search verification helpers specific to the clp-text package.""" | |||
There was a problem hiding this comment.
Just realized that we don't have actual running search tests for clp-text either. I think it'd be beneficial to add count tests to verify that things are working.
Grep uses regex queries, while clp-text uses wildcard substring queries, so maybe we need regex and wildcard translation utilities? I know there is one inside clp_json/verification/search.py, so it's crucial that we put these general utilities under the general utils directory.
There was a problem hiding this comment.
I think it'd be beneficial to add count tests to verify that things are working.
Will discuss offline
maybe we need regex and wildcard translation utilities?
This module already has these utilities; did I misunderstand you?
it's crucial that we put these general utilities under the general utils directory.
Like the methods used in integration-tests/tests/package_tests/clp_text/verification/search.py, I don't see a lot of opportunity for reuse. For example, clp-json search verification does not use grep at all, and clp-text uses only grep.
2 participants
Description
This PR adds the action and verification infrastructure for testing package search.
Checklist
breaking change.
Validation performed
Tested on development branch; all tests pass.
Summary by CodeRabbit