feat(integration-tests): Add search action and verification infra.#2235
feat(integration-tests): Add search action and verification infra.#2235quinntaylormitchell wants to merge 6 commits into
Conversation
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 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
| ) | ||
|
|
||
| args: SearchArgs = _construct_args(clp_package, dataset, search_type, wildcard_query) | ||
| return ClpAction(cmd=args.to_cmd(), args=args) |
There was a problem hiding this comment.
| return ClpAction(cmd=args.to_cmd(), args=args) | |
| return ClpAction.from_args(args) |
| dataset.dataset_name, | ||
| ) | ||
|
|
||
| args: SearchArgs = _construct_args(clp_package, dataset, search_type, wildcard_query) |
There was a problem hiding this comment.
again, feels like there's no need to create a helper for what feels like the meat of search_clp_package.
| returncode_result = action.verify_returncode() | ||
| if not returncode_result: | ||
| return returncode_result |
There was a problem hiding this comment.
| returncode_result = action.verify_returncode() | |
| if not returncode_result: | |
| return returncode_result | |
| result = action.verify_returncode() | |
| if not result: | |
| return result |
| "single_match_wildcard_query": "\"detail\":\"Roll program complete, heads down attitude achieved for ascent\"", | ||
| "single_match_file": "sts-135-2011-07-08.jsonl" |
There was a problem hiding this comment.
This is starting to feel like these entries are more like test cases instead of metadata. Why don't we inline these queries and query results in test code directly?
Also each time you add something, you'd have to update the metadata format documentation.
| class ClpPackageSearchType(Enum): | ||
| """Possible search types.""" | ||
|
|
||
| BASIC = auto() | ||
| FILE_PATH = auto() | ||
| IGNORE_CASE = auto() | ||
| COUNT_RESULTS = auto() | ||
| COUNT_BY_TIME = auto() | ||
| TIME_RANGE = auto() |
There was a problem hiding this comment.
I don't really like different test case flavours hiding away as an Enum class inside a utils folder. Without looking at your existing infra, the clearest design to me would look something like:
@pytest.mark.parametrize(
"clp_package",
[CLP_TEXT_MODE, CLP_JSON_MODE],
indirect=True,
ids=[CLP_TEXT_MODE.mode_name, CLP_JSON_MODE.mode_name],
)
class TestPackageSearch:
def test_basic_search(self, clp_package: ClpPackage) -> None:
...
def test_file_path_search(self, clp_package: ClpPackage) -> None:
...
def test_ignore_case_search(self, clp_package: ClpPackage) -> None:
...
def test_count_results_search(self, clp_package: ClpPackage) -> None:
...
def test_count_by_time_search(self, clp_package: ClpPackage) -> None:
...
def test_time_range_search(self, clp_package: ClpPackage) -> None:
...
There was a problem hiding this comment.
However, now you already have a split file design. The split file design is sound if the package behaviors are meaningfully different and you want each file to document a specific package flavor.
But it becomes weak if the tests end up looking like:
test_clp_text.py→ calls a generic helpertest_clp_json.py→ calls the same generic helper
with all meaningful behavior hidden behind shared utilities.
A sound split file design with search tests would look more like:
-
test_clp_text.py- Uses
text_multifile - Shows CLP/text specific command expectations
- Does not pass
--dataset - Does not pass
--timestamp-key - Verifies plain text search behavior
- Uses
-
test_clp_json.py- Uses
json_multifile - Shows CLP-S/JSON specific command expectations
- Passes
--dataset - Passes
--timestamp-key - Verifies structured search behavior and unstructured + log-converter.
- Uses
There is an abundance of over-abstracting the package specific behavior that I'm seeing in these PRs.
A rule of thumb for this test suite could be:
- Keep package lifecycle fixtures shared.
- Keep low-level action, logging, and result helpers shared.
- Keep subprocess calling shared.
However, keep package-specific command construction and expectations visible in test_clp_text.py and test_clp_json.py. That preserves the value of having separate test modules.
| if self.metadata.single_match_file not in self.metadata.file_names: | ||
| err_msg = ( | ||
| f"`single_match_file` '{self.metadata.single_match_file}' is not listed in" | ||
| " `file_names`." | ||
| ) | ||
| raise ValueError(err_msg) |
There was a problem hiding this comment.
Technically, there's no need to verify this, because if the single_match_file contains the wrong value, the search test will fail anyway.
This touches upon my previous comment on what should be metadata and what should merely be test case constants/inputs/outputs.
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