fix: added conditions to ensure group filter actually filters index plot output #296
Conversation
WalkthroughThe test for generating an index plot with group filtering was updated to use different column values and now includes assertions that explicitly verify the plot only contains the specified groups after filtering. No changes were made to any public or exported entities. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as test_generates_index_plot_with_group_filter
participant PlotFunc as Plot Function
Test->>PlotFunc: Call with index_col="region", value_to_index="North", include_only_groups=["A", "B"]
PlotFunc-->>Test: Return plot object
Test->>Test: Extract y-axis tick labels from plot
Test->>Test: Assert tick labels ⊆ {"A", "B"} and at least one group present
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/plots/test_index.py (1)
266-273: 💡 Verification agent🧩 Analysis chain
Baseline region missing for one of the filtered groups – test may silently pass with incomplete coverage
index_colis now"region"andvalue_to_indexis"North", but in thetest_datafixture only the rows for category"A"haveregion == "North".
Category"B"never contains the baseline region, so its index will beNaNand most likely gets dropped insideplot().
The assertion that follows merely checks that some groups were plotted (subset check), therefore the test can still succeed even if category"B"is absent – i.e. the group filter is not really exercised for"B".Consider extending/overriding the fixture inside this test so every filtered group has at least one
"North"row, or switch back to anindex_colthat is present for all groups.
Example quick fix (override inside the test):df = test_data.copy() df.loc[df["category"] == "B", "region"] = "North" # ensure baseline present
🏁 Script executed:
#!/bin/bash set -e echo "Searching for definition of 'test_data' fixture..." rg "@pytest.fixture" -A 3 -n --color=never | grep -C 3 "test_data" echo "---" echo "Searching all references to 'test_data'..." rg "test_data" -n --color=neverLength of output: 6966
🏁 Script executed:
#!/bin/bash sed -n '180,260p' tests/plots/test_index.pyLength of output: 2601
🏁 Script executed:
#!/bin/bash set -e sed -n '260,340p' tests/plots/test_index.pyLength of output: 3043
Ensure baseline region for all filtered groups in the group‐filter test
The current fixture assigns
"North"only to category"A", so when you filter to["A","B"]withvalue_to_index="North", category"B"lacks a baseline and will be dropped—yet the test only asserts a non‐empty subset, masking this gap.Locations needing adjustment:
- tests/plots/test_index.py:
TestIndexPlot.test_generates_index_plot_with_group_filter(around lines 266–273)Suggested change:
def test_generates_index_plot_with_group_filter(self, test_data): """Test that the function generates an index plot with a group filter applied.""" - df = test_data + df = test_data.copy() + # ensure baseline region 'North' exists for category 'B' + df.loc[df["category"] == "B", "region"] = "North" result_ax = plot( df, value_col="sales", group_col="category", index_col="region", value_to_index="North", include_only_groups=["A", "B"], )This guarantees both
"A"and"B"have a"North"index to validate the group filter properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/plots/test_index.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pre-Commit
| # Verify that only the filtered groups appear in the plot | ||
| y_labels = [label.get_text() for label in result_ax.get_yticklabels()] | ||
| plotted_groups = set(y_labels) | ||
| expected_groups = {"A", "B"} | ||
|
|
||
| assert plotted_groups.issubset(expected_groups), ( | ||
| f"Found groups {plotted_groups - expected_groups} that should have been filtered out. " | ||
| f"Expected only groups from {expected_groups}." | ||
| ) | ||
|
|
||
| assert len(plotted_groups) > 0, "No groups were plotted - filtering may have removed all data." | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assertion too weak – should require exact group match
At the moment you only verify that the plotted groups are a subset of { "A", "B" }, which would still pass if only "A" appears.
Since the purpose is to guarantee the filter shows only the requested groups and that both groups survived the pipeline, assert equality:
-# Verify that only the filtered groups appear in the plot
-y_labels = [label.get_text() for label in result_ax.get_yticklabels()]
-plotted_groups = set(y_labels)
-expected_groups = {"A", "B"}
-
-assert plotted_groups.issubset(expected_groups), (
- f"Found groups {plotted_groups - expected_groups} that should have been filtered out. "
- f"Expected only groups from {expected_groups}."
-)
-
-assert len(plotted_groups) > 0, "No groups were plotted - filtering may have removed all data."
+# Verify that exactly the requested groups appear in the plot
+y_labels = [label.get_text() for label in result_ax.get_yticklabels()]
+plotted_groups = set(y_labels)
+expected_groups = {"A", "B"}
+
+assert plotted_groups == expected_groups, (
+ f"Expected groups {expected_groups}, got {plotted_groups}."
+)This both guarantees no extra groups slip through and that none of the requested ones were dropped.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Verify that only the filtered groups appear in the plot | |
| y_labels = [label.get_text() for label in result_ax.get_yticklabels()] | |
| plotted_groups = set(y_labels) | |
| expected_groups = {"A", "B"} | |
| assert plotted_groups.issubset(expected_groups), ( | |
| f"Found groups {plotted_groups - expected_groups} that should have been filtered out. " | |
| f"Expected only groups from {expected_groups}." | |
| ) | |
| assert len(plotted_groups) > 0, "No groups were plotted - filtering may have removed all data." | |
| # Verify that exactly the requested groups appear in the plot | |
| y_labels = [label.get_text() for label in result_ax.get_yticklabels()] | |
| plotted_groups = set(y_labels) | |
| expected_groups = {"A", "B"} | |
| assert plotted_groups == expected_groups, ( | |
| f"Expected groups {expected_groups}, got {plotted_groups}." | |
| ) |
🤖 Prompt for AI Agents
In tests/plots/test_index.py around lines 277 to 288, the assertion currently
checks if the plotted groups are a subset of {"A", "B"}, which allows cases
where only one group appears. Change the assertion to require exact equality
between plotted_groups and expected_groups to ensure that both "A" and "B" are
present and no extra groups appear. Replace the issubset check with an equality
check and update the assertion message accordingly.
Summary by CodeRabbit