Skip to content

test: harden platform recommendation coverage#36

Merged
ternaus merged 1 commit into
mainfrom
codex/platform-recommendation-test-coverage
May 12, 2026
Merged

test: harden platform recommendation coverage#36
ternaus merged 1 commit into
mainfrom
codex/platform-recommendation-test-coverage

Conversation

@ternaus
Copy link
Copy Markdown
Owner

@ternaus ternaus commented May 12, 2026

Summary by Sourcery

Strengthen validation of platform recommendation data and refactor LaTeX table row generation.

Enhancements:

  • Extract LaTeX table row formatting into a dedicated helper used by platform recommendation table generation.

Tests:

  • Expand platform recommendation table test to validate zero-skip choices per platform, ensure sorted throughput, and require worker-count annotations.
  • Add a dedicated test enforcing that platform recommendations must include at least three zero-skip DataLoader choices.

Copilot AI review requested due to automatic review settings May 12, 2026 08:01
@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented May 12, 2026

Reviewer's Guide

This PR strengthens tests around the platform recommendation table generation by asserting structural properties of the generated rows instead of hard-coded strings, adds a dedicated failure-mode test for insufficient zero-skip choices, and slightly refactors LaTeX table row generation into a helper for better reuse and clarity.

Flow diagram for platform recommendation LaTeX table generation

flowchart TD
    A[dl DataFrame] --> B[_platform_recommendation_rows]
    B --> C[rows list]
    C --> D[_latex_table_row for each row]
    D --> E[LaTeX body string]
    E --> F[generate_platform_recommendation_table writes to dest Path]
Loading

File-Level Changes

Change Details Files
Harden platform recommendation table test to validate structure and ordering of recommendation rows instead of relying on hard-coded expected strings.
  • Import the internal _platform_recommendation_rows helper into tests to inspect the generated rows directly.
  • Replace the previous list of hard-coded LaTeX string expectations with assertions on per-platform row count, uniqueness of the top three choices, descending throughput ordering, and presence of worker count annotations in each choice.
  • Compute throughput values by parsing the generated choice strings and comparing them to a sorted copy to ensure performance ordering is correct.
tests/test_paper_assets.py
Add a regression test ensuring _platform_recommendation_rows raises a clear error when there are fewer than three zero-skip DataLoader choices for a platform.
  • Introduce a new test that builds a minimal synthetic DataFrame with only one zero-skip entry and one non-zero-skip entry for a single platform.
  • Assert that calling _platform_recommendation_rows with this DataFrame raises ValueError with the expected error message fragment.
tests/test_paper_assets.py
Refactor LaTeX table row string construction into a reusable helper to simplify generate_platform_recommendation_table.
  • Introduce a _latex_table_row(row: list[str]) helper that formats a list of cell values into a LaTeX table row string with the appropriate indentation and line terminator.
  • Update generate_platform_recommendation_table to build the table body using _latex_table_row instead of inlining the string join logic inside a comprehension.
tools/paper_assets.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • The new test that infers throughput ordering by parsing the formatted choice strings (choice.split(...)) is somewhat brittle; consider asserting on structured data (e.g., the underlying rows before formatting or a helper that returns structured choices) so future format changes don’t silently break intent.
  • Since _latex_table_row encodes a specific LaTeX formatting convention used in multiple places, consider adding a small unit test around it or centralizing any related escaping/formatting logic, so changes to the table row format are easier to manage consistently.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new test that infers throughput ordering by parsing the formatted choice strings (`choice.split(...)`) is somewhat brittle; consider asserting on structured data (e.g., the underlying rows before formatting or a helper that returns structured choices) so future format changes don’t silently break intent.
- Since `_latex_table_row` encodes a specific LaTeX formatting convention used in multiple places, consider adding a small unit test around it or centralizing any related escaping/formatting logic, so changes to the table row format are easier to manage consistently.

## Individual Comments

### Comment 1
<location path="tests/test_paper_assets.py" line_range="157-165" />
<code_context>
-    ]
-    for choice in expected_choices:
-        assert choice in text
+    rows = _platform_recommendation_rows(load_dataloader_results(root))
+    assert len(rows) == len(PAPER_PLATFORMS)
+    for row in rows:
+        choices = row[1:4]
+        assert len(choices) == 3
+        assert len(set(choices)) == 3
+        throughputs = [int(choice.split(": ", 1)[1].split(" img/s", 1)[0]) for choice in choices]
+        assert throughputs == sorted(throughputs, reverse=True)
+        assert all("$w=" in choice for choice in choices)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert which platforms are present (and optionally their order) in the recommendation rows.

As written, the test would still pass if `_platform_recommendation_rows` dropped, duplicated, or reordered platforms as long as the row count stayed the same. To guard against that, assert the platform column exactly matches `PAPER_PLATFORMS`, e.g. `assert [row[0] for row in rows] == list(PAPER_PLATFORMS)`.

```suggestion
    rows = _platform_recommendation_rows(load_dataloader_results(root))
    assert len(rows) == len(PAPER_PLATFORMS)
    # Ensure that each row corresponds to the expected platform, in order
    assert [row[0] for row in rows] == list(PAPER_PLATFORMS)
    for row in rows:
        choices = row[1:4]
        assert len(choices) == 3
        assert len(set(choices)) == 3
        throughputs = [int(choice.split(": ", 1)[1].split(" img/s", 1)[0]) for choice in choices]
        assert throughputs == sorted(throughputs, reverse=True)
        assert all("$w=" in choice for choice in choices)
```
</issue_to_address>

### Comment 2
<location path="tests/test_paper_assets.py" line_range="168-177" />
<code_context>
+        assert all("$w=" in choice for choice in choices)
+
+
+def test_platform_recommendation_rows_requires_three_zero_skip_choices() -> None:
+    platform = PAPER_PLATFORMS[0]
+    dl = pd.DataFrame(
+        [
+            {
+                "platform": platform,
+                "library": "opencv",
+                "num_workers": 0,
+                "images_per_second": 100.0,
+                "cpu_brand": "Synthetic CPU",
+            },
+            {
+                "platform": platform,
+                "library": "imageio",
+                "num_workers": 2,
+                "images_per_second": 90.0,
+                "cpu_brand": "Synthetic CPU",
+            },
+        ],
+    )
+
+    with pytest.raises(ValueError, match="fewer than three zero-skip DataLoader choices"):
+        _platform_recommendation_rows(dl)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a positive test that explicitly checks non-zero-skip rows are ignored when enough zero-skip rows exist.

To complement this negative-path test, please add a positive-path unit test that builds a small `DataFrame` with at least three zero-skip entries plus some higher-throughput non-zero-skip entries, and asserts that `_platform_recommendation_rows` selects only the zero-skip rows. This will lock in the intended filtering behavior and guard against regressions where non-zero-skip rows start being chosen.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread tests/test_paper_assets.py Outdated
Comment on lines +168 to +177
def test_platform_recommendation_rows_requires_three_zero_skip_choices() -> None:
platform = PAPER_PLATFORMS[0]
dl = pd.DataFrame(
[
{
"platform": platform,
"library": "opencv",
"num_workers": 0,
"images_per_second": 100.0,
"cpu_brand": "Synthetic CPU",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a positive test that explicitly checks non-zero-skip rows are ignored when enough zero-skip rows exist.

To complement this negative-path test, please add a positive-path unit test that builds a small DataFrame with at least three zero-skip entries plus some higher-throughput non-zero-skip entries, and asserts that _platform_recommendation_rows selects only the zero-skip rows. This will lock in the intended filtering behavior and guard against regressions where non-zero-skip rows start being chosen.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the platform recommendation table tests by asserting structural and ordering guarantees (three distinct, zero-skip choices per platform, ordered by throughput) rather than relying on brittle, exact throughput strings.

Changes:

  • Refactors LaTeX row formatting into a small helper for reuse/clarity.
  • Updates the platform recommendation table test to validate choice count/uniqueness/ordering and worker annotation.
  • Adds a unit test to ensure _platform_recommendation_rows raises when fewer than three zero-skip choices are available.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tools/paper_assets.py Adds _latex_table_row() helper and uses it when building the LaTeX table body.
tests/test_paper_assets.py Strengthens platform recommendation coverage with structural assertions and a new error-path test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ternaus ternaus force-pushed the codex/platform-recommendation-test-coverage branch from f43719f to 59cd298 Compare May 12, 2026 08:09
@ternaus ternaus merged commit 1cecc81 into main May 12, 2026
6 checks passed
@ternaus ternaus deleted the codex/platform-recommendation-test-coverage branch May 12, 2026 08:16
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.

2 participants