Skip to content

test(metrics): add coverage for metrics timing decorator#1003

Open
mnadzam wants to merge 1 commit intopython-wheel-build:mainfrom
mnadzam:tests_metrics
Open

test(metrics): add coverage for metrics timing decorator#1003
mnadzam wants to merge 1 commit intopython-wheel-build:mainfrom
mnadzam:tests_metrics

Conversation

@mnadzam
Copy link
Copy Markdown

@mnadzam mnadzam commented Apr 1, 2026

Closes #978

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 1, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: da814a28-75c5-4092-9088-1d22c52ff1bc

📥 Commits

Reviewing files that changed from the base of the PR and between 59b813f and 415ece4.

📒 Files selected for processing (1)
  • tests/test_metrics.py
✅ Files skipped from review due to trivial changes (1)
  • tests/test_metrics.py

📝 Walkthrough

Walkthrough

Adds tests/tests_metrics.py (new) which provides pytest coverage for fromager.metrics. It defines several functions decorated with @metrics.timeit to exercise timing storage and descriptions on a provided context.WorkContext, asserting that timing entries are written to tmp_context.time_store only when both a requirement and a resolvable version source exist (explicit version arg or Version in the return). Tests verify descriptions stored in tmp_context.time_description_store, that original return values are preserved, that exceptions propagate without writing timing data, that metrics.summarize() logs expected INFO output when data exists and none when empty, and unit-tests _extract_version_from_return() across None, Version, tuples, and non-iterable inputs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the primary change: adding pytest coverage for the metrics timing decorator.
Description check ✅ Passed The description references issue #978, which directly relates to the PR's objective of adding test coverage for metrics.py.
Linked Issues check ✅ Passed The PR implements all four acceptance criteria from #978: timeit() timing storage, timeit() exception propagation, summarize() logging, and _extract_version_from_return() behavior across variants.
Out of Scope Changes check ✅ Passed All changes are scoped to test coverage for metrics.py as required; no unrelated modifications to source code or other modules detected.

✏️ 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.

@mergify mergify bot added the ci label Apr 1, 2026
@mnadzam mnadzam marked this pull request as ready for review April 1, 2026 11:10
@mnadzam mnadzam requested a review from a team as a code owner April 1, 2026 11:10
Copy link
Copy Markdown

@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: 2

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

Inline comments:
In `@tests/test_metrics.py`:
- Around line 128-144: Add a new unit test in tests/test_metrics.py that calls
metrics._extract_version_from_return(None) and asserts it returns None to cover
the acceptance criterion for issue `#978`; ensure the test function name is unique
(e.g., test_extract_version_none) and follows the same style as the other tests
so it runs with the existing test suite and exercises the code path where
iterating over None would raise a TypeError.
- Around line 31-37: The helper function _test_raises used under the
metrics.timeit decorator must accept a version parameter so the decorator's
storage path (which only runs when both req and version are present) can be
exercised; update the _test_raises signature to include version: str | None =
None (or the project's Version type if different) and ensure the function
accepts and ignores this parameter while still raising the RuntimeError so the
test can pass a version and verify that exceptions prevent storage.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5f0dfdb8-5fec-4664-9c4e-b7c1d569f8bc

📥 Commits

Reviewing files that changed from the base of the PR and between aec9c9c and 59b813f.

📒 Files selected for processing (1)
  • tests/test_metrics.py

Closes python-wheel-build#978

Signed-off-by: Marcel Nadzam <mnadzam@redhat.com>

Co-Authored-By: Cursor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: add coverage for metrics timing decorator (metrics.py)

1 participant