test: harden platform recommendation coverage#36
Conversation
Reviewer's GuideThis 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 generationflowchart 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]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
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_rowencodes 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_rowsraises 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.
f43719f to
59cd298
Compare
Summary by Sourcery
Strengthen validation of platform recommendation data and refactor LaTeX table row generation.
Enhancements:
Tests: