Skip to content

fix: only filter by invocations_rank_index when no invocation_id is specified#2143

Merged
haritamar merged 2 commits intomasterfrom
devin/1773329847-fix-invocation-id-filter
Mar 12, 2026
Merged

fix: only filter by invocations_rank_index when no invocation_id is specified#2143
haritamar merged 2 commits intomasterfrom
devin/1773329847-fix-invocation-id-filter

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot commented Mar 12, 2026

Summary

Fixes a bug where edr report --select invocation_id:XXXX only worked for the most recent test invocation. For any older invocation, the report would show 0 tests.

Root cause: In TestsAPI.get_test_results(), the invocations_rank_index == 1 filter ran unconditionally after the invocation_id filter. When a non-latest invocation was selected, results were correctly filtered to that invocation, then immediately discarded because invocations_rank_index != 1.

Fix: Move the invocations_rank_index == 1 filter into an else block so it only applies when no specific invocation_id is provided. This matches the already-correct pattern in get_test_results_summary() in the same class.

The remaining diff lines are cosmetic black reformatting (no logic changes).

Review & Testing Checklist for Human

  • Verify the logic: when invocation_id is provided, skipping the invocations_rank_index == 1 filter won't cause duplicate/unexpected rows for the same test within a single invocation
  • Test edr report --select invocation_id:<non_latest_id> against a warehouse with historical test data and confirm tests now appear
  • Confirm edr report (without --select invocation_id) still behaves correctly (only shows latest invocation results)

Notes

Summary by CodeRabbit

  • Bug Fixes

    • Fixed test result filtering so results reflect whether a specific invocation is requested; when no invocation is specified, only primary-ranked results are returned.
  • Refactor

    • Improved clarity and structure of the test-result processing logic for easier maintenance.

…pecified

When using 'edr report --select invocation_id:XXXX', the report showed 0
tests for non-latest invocations because the invocations_rank_index == 1
filter ran unconditionally after the invocation_id filter, removing all
results where the selected invocation wasn't the most recent one.

This mirrors the existing correct pattern in get_test_results_summary().

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@github-actions
Copy link
Copy Markdown
Contributor

👋 @devin-ai-integration[bot]
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f6cc4278-689d-476d-b238-787e5ec46c3e

📥 Commits

Reviewing files that changed from the base of the PR and between b9635a8 and 2c06b45.

📒 Files selected for processing (1)
  • elementary/monitor/api/tests/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • elementary/monitor/api/tests/tests.py

📝 Walkthrough

Walkthrough

get_test_results now applies the invocations_rank_index == 1 filter only when invocation_id is not provided; when invocation_id is present, results are filtered by that ID without forcing rank-index == 1. Other reformatting changed expression layout only.

Changes

Cohort / File(s) Summary
Conditional Filtering Logic
elementary/monitor/api/tests/tests.py
get_test_results control flow changed: the invocations_rank_index == 1 filter is moved into the branch for missing invocation_id, so it is no longer applied unconditionally. Minor multi-line expression reformatting elsewhere in the file only affects formatting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nibble code with careful paws,

When IDs hop in, the rank withdraws.
A tidy branch, a focused view,
Fewer results, but clearer too.
Hooray — the tests now filter true!

🚥 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: moving the invocations_rank_index filter to only apply when no invocation_id is specified, which is the core fix for the reported bug.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch devin/1773329847-fix-invocation-id-filter
📝 Coding Plan for PR comments
  • Generate coding plan

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

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: 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 `@elementary/monitor/api/tests/tests.py`:
- Around line 431-438: The two timestamp assignments are reversed: ensure
latest_run_time is the local-time ISO string and latest_run_time_utc is the UTC
ISO string to match _get_test_metadata_from_test_result_db_row(); swap the
values so latest_run_time uses latest_run_datetime.isoformat() or
latest_run_datetime.astimezone(tz.tzlocal()).isoformat() for local time and
latest_run_time_utc uses latest_run_datetime.astimezone(tz.tzutc()).isoformat()
(or datetime.timezone.utc) for UTC, updating the assignments for latest_run_time
and latest_run_time_utc accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f0a7320f-343f-44b7-9bf8-a571291ddfab

📥 Commits

Reviewing files that changed from the base of the PR and between 63150b9 and b9635a8.

📒 Files selected for processing (1)
  • elementary/monitor/api/tests/tests.py

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
@haritamar haritamar merged commit ba9e92a into master Mar 12, 2026
23 checks passed
@haritamar haritamar deleted the devin/1773329847-fix-invocation-id-filter branch March 12, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant