Add e2e pytest test for inspect command #2269
Conversation
telemaco
requested review from
bmahabirbu,
cgruver,
engelmi,
jhjaggars,
maxamillion,
rhatdan and
swarajpande5
as code owners
Reviewer's GuideAdds new end-to-end pytest coverage for the File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @telemaco, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the testing infrastructure by migrating the system test for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The tests assert a large number of exact metadata values (counts, tensor sizes, model properties) for remote models, which makes them very brittle to model or metadata updates; consider either pinning specific model versions or relaxing these assertions to only check for structural properties and a few key fields.
- Similarly, the expected metadata counts for GGUF and safetensors models (e.g., 23, 201, 288) are tightly coupled to the current artifacts; using more tolerant checks (e.g.,
>=or presence of representative keys) would reduce the likelihood of spurious failures when models change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests assert a large number of exact metadata values (counts, tensor sizes, model properties) for remote models, which makes them very brittle to model or metadata updates; consider either pinning specific model versions or relaxing these assertions to only check for structural properties and a few key fields.
- Similarly, the expected metadata counts for GGUF and safetensors models (e.g., 23, 201, 288) are tightly coupled to the current artifacts; using more tolerant checks (e.g., `>=` or presence of representative keys) would reduce the likelihood of spurious failures when models change.
## Individual Comments
### Comment 1
<location> `test/e2e/test_inspect.py:46-52` </location>
<code_context>
+ pytest.param(GGUF_MODEL, False, ["Registry"], "ollama", id="gguf_inspect_registry"),
+ pytest.param(GGUF_MODEL, False, ["Format"], "GGUF", id="gguf_inspect_format"),
+ pytest.param(GGUF_MODEL, False, ["Version"], "3", id="gguf_inspect_version"),
+ pytest.param(
+ GGUF_MODEL, False, ["Endianness"], "0" if sys.byteorder == "little" else "1", id="gguf_inspect_endianness"
+ ),
+ pytest.param(GGUF_MODEL, False, ["Metadata"], "23", id="gguf_inspect_metadata_count"),
+ pytest.param(GGUF_MODEL, False, ["Tensors"], "201", id="gguf_inspect_tensors_count"),
+ pytest.param(
+ GGUF_MODEL,
+ False,
+ ["Path"],
+ Path(".*", "store", "ollama", "library", "tinyllama", ".*"),
+ id="gguf_inspect_path",
+ ),
</code_context>
<issue_to_address>
**suggestion (testing):** The `Path(".*", ...)` expectation is mixing regex semantics with filesystem paths and may behave unexpectedly on Windows or if separators change.
Here this value is converted to `str` and fed to `re.match`, so on Windows the pattern will contain backslashes that are treated as escapes, making the regex brittle and tied to one directory layout. Consider either using a plain regex string with explicit separators (e.g. `r".*/store/ollama/library/tinyllama/.*"`) or, alternatively, parsing the JSON value as a `Path` and asserting on its components (e.g. that `"store"`, `"ollama"`, `"library"`, `"tinyllama"` are present in `Path(value).parts`).
```suggestion
pytest.param(
GGUF_MODEL,
False,
["Path"],
r".*store[\\/]+ollama[\\/]+library[\\/]+tinyllama.*",
id="gguf_inspect_path",
),
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive suite of end-to-end tests for the ramalama inspect command using pytest. The tests are well-structured, covering various model types (GGUF, safetensors), command-line flags (--all, --json), and options (--get). The use of pytest.parametrize is effective for testing multiple scenarios concisely. My review includes suggestions to improve the robustness of a test case to ensure it works across different operating systems and to enhance the efficiency of another test to speed up the test suite.
telemaco
force-pushed
the
e2e-pytest-inspect-cmd
branch
2 times, most recently
from
d1b71cb to
93e09b5
Compare
- Migrate to e2e pytest the `test/system/100-inspect.bats` bat test. Signed-off-by: Roberto Majadas <[email protected]>
telemaco
force-pushed
the
e2e-pytest-inspect-cmd
branch
from
93e09b5 to
6e0191e
Compare
|
This is ready @olliewalsh @rhatdan |
|
|
||
| import pytest | ||
|
|
||
| GGUF_MODEL = "ollama://tinyllama" |
There was a problem hiding this comment.
Would be best to use a specific tag, in case latest changes e.g ollama://tinyllama:1.1b
There was a problem hiding this comment.
This is coming from the bats test though, so can address it in a follow-up PR.
2 participants
test/system/100-inspect.batsbat test.Summary by Sourcery
Add end-to-end pytest coverage for the
ramalama inspectcommand across GGUF and safetensors models, including error handling and metadata queries.Tests:
ramalama inspectJSON output for GGUF and safetensors models with and without the--allflag.ramalama inspectbehavior when inspecting a non-existent model, asserting the correct error code and message.ramalama inspect --getto validate retrieval of specific and all GGUF metadata fields.