Drop unused ChartAxis members and cover the empty-series guard - #962
Open
ericgriffin wants to merge 2 commits into
Open
Drop unused ChartAxis members and cover the empty-series guard#962ericgriffin wants to merge 2 commits into
ericgriffin wants to merge 2 commits into
Conversation
Codecov flagged 11 uncovered lines in chart_axis.dart. Eight were operator ==, hashCode and toString, which nothing calls: ChartAxis is built fresh inside build() and never compared or printed. A ninth was the trailing return in _nextStepUp, which is unreachable because _magnitudeOf floors the exponent, so normalized is always in [1, 10) and the 10 ending both ladders always matches. Testing either would have been coverage theatre, so both are removed instead; _nextStepUp now uses firstWhere without an orElse, which throws loudly if that invariant ever breaks rather than silently returning a plausible wrong step. The remaining gap was real behaviour: forTrend handed an empty series still has to produce a drawable axis. That one gets a test. chart_axis.dart is now at 48/48 lines.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to #948 focused on eliminating dead ChartAxis API surface and closing the only meaningful unit-test gap: ensuring ChartAxis.forTrend still produces a valid/drawable axis when given an empty series.
Changes:
- Added a unit test covering
ChartAxis.forTrend(const [])to ensure bounds and tick alignment remain valid for empty input. - Simplified
_nextStepUpby removing an unreachable fallback path and relying onfirstWhereto enforce invariants. - Removed unused
ChartAxisoverrides (==,hashCode,toString) that have no callers in the codebase.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/features/statistics/presentation/widgets/chart_axis_test.dart | Adds coverage for the empty-series guard to ensure the axis remains drawable and tick-aligned. |
| lib/features/statistics/presentation/widgets/chart_axis.dart | Removes unused overrides and refactors _nextStepUp to rely on a stricter invariant check. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
|
📦 Build artifacts for this PR · commit
Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the Updated automatically on each push. |
Review on this PR caught a real crash that the previous commit introduced. Removing the trailing return in _nextStepUp was justified by the claim that normalized is always in [1, 10), and that claim is false: math.log(1000) / math.ln10 is 2.9999999999999996, so flooring it yields 2, the magnitude comes out as 100 instead of 1000, and normalized lands on exactly 10. The same holds at 1e6, 1e9 and 1e12. Both step ladders end at 10, so the search for a wider rung matched nothing and threw "Bad state: No element" while snapping the axis. A sweep found 22 reproducing inputs through the public API, among them ChartAxis.forTrend([1407.7040137891763, 5630.816055156705]). Fix the invariant at its source rather than restoring a fallback that would hide a broken contract: _magnitudeOf now corrects the exponent in either direction, so value / magnitude genuinely sits in [1, 10) and _nextStepUp's firstWhere is safe. Covered by the two reproducing cases and a sweep over spans from 1 to 200000 asserting each axis is non-empty and tick-aligned; both fail against the previous commit with the exact StateError.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #948, which merged before this landed on the branch. Addresses the
Codecov report on that PR: patch coverage 84.06%, with 11 lines uncovered in the
new
chart_axis.dart.Reading the gap rather than just filling it, only one of the three causes was
actually untested behaviour:
8 lines were dead API.
operator ==,hashCodeandtoStringwere addedspeculatively.
ChartAxisis built fresh insidebuild()and is nevercompared, hashed or printed anywhere in the codebase. Removed.
1 line looked structurally unreachable, and was not. The trailing
return 10 * magnitudein_nextStepUpwas removed on the grounds thatnormalizedis always in[1, 10). Review on this PR caught that the claimis false, and the removal turned a graceful degradation into a crash. See the
correction below.
The genuine gap was
ChartAxis.forTrendhanded an empty series, whichstill has to produce a drawable axis. That one gets a test.
Writing tests for the dead API would have been coverage theatre.
chart_axis.dartis now at 52/52 lines.
Correction: a crash introduced and fixed within this PR
The second bullet above was wrong, and Copilot's review caught it.
math.log(1000) / math.ln10is2.9999999999999996, so flooring it yields 2,_magnitudeOfreturns 100 instead of 1000, andnormalizedlands on exactly10. The same holds at 1e6, 1e9 and 1e12. Both step ladders end at 10, so thesearch for a wider rung matched nothing and threw
Bad state: No elementwhilesnapping the axis. A sweep over the public API found 22 reproducing inputs, for
instance
ChartAxis.forTrend([1407.7040137891763, 5630.816055156705]).The deleted fallback had been quietly absorbing this: it returned
10 * magnitude, which equalled the current interval, so the caller'sif (wider <= interval) breakended the loop cleanly.Fixed at the source rather than by restoring the fallback:
_magnitudeOfnowcorrects the exponent in either direction, so
value / magnitudegenuinely sitsin
[1, 10)and thefirstWhereis safe. Restoring the fallback would have kepta broken contract alive behind a plausible-looking default.
Changes
operator ==,hashCodeandtoStringfromChartAxis._nextStepUpto a singlefirstWhereexpression._magnitudeOfsovalue / magnitudereally is in[1, 10), fixingthe
Bad state: No elementcrash that thefirstWhereexposed.ChartAxis.forTrend(const [])test asserting the empty-series axis isstill drawable and tick-aligned.
plus a sweep over spans from 1 to 200000 asserting every axis is non-empty and
tick-aligned. Both fail against the previous commit with that StateError.
Test Plan
flutter analyzepassesdart format .cleanchart_axis.dartline coverage 52/52flutter testfull suite passesThe removed members had no callers. The one behaviour change is the
_magnitudeOfcorrection, which fixes a crash.