Charts: Add documentation and tests for TimeSeriesForecastChart#47152
Open
annacmc wants to merge 6 commits intoadd/time-series-forecast-chartfrom
Open
Charts: Add documentation and tests for TimeSeriesForecastChart#47152annacmc wants to merge 6 commits intoadd/time-series-forecast-chartfrom
annacmc wants to merge 6 commits intoadd/time-series-forecast-chartfrom
Conversation
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
annacmc
changed the base branch from
trunk
to
add/time-series-forecast-chart
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive documentation and test coverage for the TimeSeriesForecastChart component, which was previously merged without these supporting materials. The PR includes Storybook documentation with interactive examples, API reference pages, unit tests for the useForecastData hook, and integration tests for the main component.
Changes:
- Added comprehensive Storybook documentation (index.docs.mdx) covering usage examples, customization options, accessibility, and theming
- Added API reference documentation (index.api.mdx) documenting all props, types, and interfaces
- Added 13 unit tests for the
useForecastDatahook covering data transformation, sorting, domain computation, and edge cases - Added 15 integration tests for
TimeSeriesForecastChartUnresponsivecovering rendering, legend, forecast divider, and empty data handling - Updated changelog to reflect documentation and test additions rather than feature additions
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/charts/src/charts/time-series-forecast-chart/test/time-series-forecast-chart.test.tsx | Integration tests for chart component covering empty data, rendering, forecast divider logic, legend visibility, and custom configurations |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/private/test/use-forecast-data.test.ts | Unit tests for useForecastData hook covering data transformation, sorting, band filtering, domain calculations, and edge cases |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/stories/index.docs.mdx | Comprehensive Storybook documentation including usage guides, examples, customization options, accessibility, and theming |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/stories/index.api.mdx | API reference documentation with complete prop tables and type definitions |
| projects/js-packages/charts/changelog/add-time-series-forecast-chart-docs | New changelog entry for documentation and test additions |
| projects/js-packages/charts/changelog/add-time-series-forecast-chart | Removed original feature changelog entry (likely being consolidated before release) |
Comment on lines
+1
to
+151
| import { render, screen } from '@testing-library/react'; | ||
| import { TimeSeriesForecastChartUnresponsive } from '..'; | ||
| import { GlobalChartsProvider } from '../../../providers'; | ||
|
|
||
| // Mock useElementHeight to return a non-zero height in jsdom so charts render | ||
| const mockRefCallback = jest.fn(); | ||
| jest.mock( '../../../hooks/use-element-height', () => ( { | ||
| useElementHeight: () => [ mockRefCallback, 300 ], | ||
| } ) ); | ||
|
|
||
| type TestDatum = { | ||
| date: Date; | ||
| value: number; | ||
| lower?: number | null; | ||
| upper?: number | null; | ||
| }; | ||
|
|
||
| const accessors = { | ||
| x: ( d: TestDatum ) => d.date, | ||
| y: ( d: TestDatum ) => d.value, | ||
| yLower: ( d: TestDatum ) => d.lower ?? null, | ||
| yUpper: ( d: TestDatum ) => d.upper ?? null, | ||
| }; | ||
|
|
||
| const sampleData: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 10 }, | ||
| { date: new Date( '2024-01-10' ), value: 20 }, | ||
| { date: new Date( '2024-01-15' ), value: 30, lower: 25, upper: 35 }, | ||
| { date: new Date( '2024-01-20' ), value: 40, lower: 35, upper: 45 }, | ||
| ]; | ||
|
|
||
| const defaultProps = { | ||
| data: sampleData, | ||
| accessors, | ||
| forecastStart: new Date( '2024-01-12' ), | ||
| width: 600, | ||
| height: 300, | ||
| }; | ||
|
|
||
| const renderChart = ( props = {} ) => | ||
| render( | ||
| <GlobalChartsProvider> | ||
| <TimeSeriesForecastChartUnresponsive { ...defaultProps } { ...props } /> | ||
| </GlobalChartsProvider> | ||
| ); | ||
|
|
||
| describe( 'TimeSeriesForecastChart', () => { | ||
| describe( 'Empty data', () => { | ||
| test( 'renders "No data available" text', () => { | ||
| renderChart( { data: [] } ); | ||
| expect( screen.getByText( /no data available/i ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'renders container with testid', () => { | ||
| renderChart( { data: [] } ); | ||
| expect( screen.getByTestId( 'time-series-forecast-chart' ) ).toBeInTheDocument(); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'Normal render', () => { | ||
| test( 'renders chart container with testid', () => { | ||
| renderChart(); | ||
| expect( screen.getByTestId( 'time-series-forecast-chart' ) ).toBeInTheDocument(); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'Forecast divider', () => { | ||
| test( 'present when both historical and forecast data exist', () => { | ||
| renderChart(); | ||
| expect( screen.getByTestId( 'forecast-divider' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'not present when forecastStart is after all data (historical only)', () => { | ||
| renderChart( { forecastStart: new Date( '2024-02-01' ) } ); | ||
| expect( screen.queryByTestId( 'forecast-divider' ) ).not.toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'not present when forecastStart is before all data (forecast only)', () => { | ||
| renderChart( { forecastStart: new Date( '2023-12-01' ) } ); | ||
| expect( screen.queryByTestId( 'forecast-divider' ) ).not.toBeInTheDocument(); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'Legend', () => { | ||
| test( 'hidden by default (showLegend=false)', () => { | ||
| renderChart(); | ||
| expect( screen.queryByTestId( /legend/ ) ).not.toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'visible with series labels when showLegend=true', () => { | ||
| renderChart( { showLegend: true } ); | ||
| expect( screen.getByText( 'Historical' ) ).toBeInTheDocument(); | ||
| expect( screen.getByText( 'Forecast' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'shows custom series labels', () => { | ||
| renderChart( { | ||
| showLegend: true, | ||
| seriesKeys: { historical: 'Actual', forecast: 'Predicted' }, | ||
| } ); | ||
| expect( screen.getByText( 'Actual' ) ).toBeInTheDocument(); | ||
| expect( screen.getByText( 'Predicted' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'band series does not appear in legend', () => { | ||
| renderChart( { showLegend: true } ); | ||
| expect( screen.queryByText( 'Uncertainty' ) ).not.toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'shows only Historical when forecastStart is after all data', () => { | ||
| renderChart( { | ||
| showLegend: true, | ||
| forecastStart: new Date( '2024-02-01' ), | ||
| } ); | ||
| expect( screen.getByText( 'Historical' ) ).toBeInTheDocument(); | ||
| expect( screen.queryByText( 'Forecast' ) ).not.toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'shows only Forecast when forecastStart is before all data', () => { | ||
| renderChart( { | ||
| showLegend: true, | ||
| forecastStart: new Date( '2023-12-01' ), | ||
| } ); | ||
| expect( screen.getByText( 'Forecast' ) ).toBeInTheDocument(); | ||
| expect( screen.queryByText( 'Historical' ) ).not.toBeInTheDocument(); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'Custom className', () => { | ||
| test( 'applies custom className to container', () => { | ||
| renderChart( { className: 'my-custom-chart' } ); | ||
| const container = screen.getByTestId( 'time-series-forecast-chart' ); | ||
| expect( container ).toHaveClass( 'my-custom-chart' ); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'Rendering', () => { | ||
| test( 'renders with animation prop without errors', () => { | ||
| renderChart( { animation: true } ); | ||
| expect( screen.getByTestId( 'time-series-forecast-chart' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| test( 'renders with custom dimensions', () => { | ||
| renderChart( { width: 800, height: 400 } ); | ||
| const container = screen.getByTestId( 'time-series-forecast-chart' ); | ||
| expect( container ).toBeInTheDocument(); | ||
| expect( container ).toHaveStyle( { width: '800px' } ); | ||
| expect( container ).toHaveStyle( { height: '400px' } ); | ||
| } ); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
The PR description claims there are "16 component integration tests" but there are actually only 15 test cases in this file. Please update the PR description to accurately reflect the test count.
Comment on lines
+1
to
+291
| import { renderHook } from '@testing-library/react'; | ||
| import { useForecastData } from '../use-forecast-data'; | ||
| import type { TimeSeriesForecastAccessors } from '../../types'; | ||
|
|
||
| type TestDatum = { | ||
| date: Date; | ||
| value: number; | ||
| lower?: number | null; | ||
| upper?: number | null; | ||
| }; | ||
|
|
||
| const accessors: TimeSeriesForecastAccessors< TestDatum > = { | ||
| x: d => d.date, | ||
| y: d => d.value, | ||
| yLower: d => d.lower ?? null, | ||
| yUpper: d => d.upper ?? null, | ||
| }; | ||
|
|
||
| const accessorsNoBounds: TimeSeriesForecastAccessors< TestDatum > = { | ||
| x: d => d.date, | ||
| y: d => d.value, | ||
| }; | ||
|
|
||
| describe( 'useForecastData', () => { | ||
| test( 'returns safe defaults for empty data', () => { | ||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data: [], | ||
| accessors, | ||
| forecastStart: new Date( '2024-01-15' ), | ||
| } ) | ||
| ); | ||
|
|
||
| expect( result.current.allPoints ).toEqual( [] ); | ||
| expect( result.current.historical ).toEqual( [] ); | ||
| expect( result.current.forecast ).toEqual( [] ); | ||
| expect( result.current.bandData ).toEqual( [] ); | ||
| expect( result.current.yDomain ).toEqual( [ 0, 100 ] ); | ||
| } ); | ||
|
|
||
| test( 'sorts unsorted input by date', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-20' ), value: 30 }, | ||
| { date: new Date( '2024-01-05' ), value: 10 }, | ||
| { date: new Date( '2024-01-10' ), value: 20 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors: accessorsNoBounds, | ||
| forecastStart: new Date( '2024-02-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| const dates = result.current.allPoints.map( p => p.date.getTime() ); | ||
| expect( dates ).toEqual( [ | ||
| new Date( '2024-01-05' ).getTime(), | ||
| new Date( '2024-01-10' ).getTime(), | ||
| new Date( '2024-01-20' ).getTime(), | ||
| ] ); | ||
| } ); | ||
|
|
||
| test( 'transition point at forecastStart appears in both historical and forecast', () => { | ||
| const forecastStart = new Date( '2024-01-10' ); | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 10 }, | ||
| { date: new Date( '2024-01-10' ), value: 20 }, | ||
| { date: new Date( '2024-01-15' ), value: 30 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { data, accessors: accessorsNoBounds, forecastStart } ) | ||
| ); | ||
|
|
||
| const historicalDates = result.current.historical.map( p => p.date.getTime() ); | ||
| const forecastDates = result.current.forecast.map( p => p.date.getTime() ); | ||
|
|
||
| expect( historicalDates ).toContain( forecastStart.getTime() ); | ||
| expect( forecastDates ).toContain( forecastStart.getTime() ); | ||
| } ); | ||
|
|
||
| test( 'historical only when forecastStart is after all data', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 10 }, | ||
| { date: new Date( '2024-01-10' ), value: 20 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors: accessorsNoBounds, | ||
| forecastStart: new Date( '2024-02-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| expect( result.current.historical ).toHaveLength( 2 ); | ||
| expect( result.current.forecast ).toHaveLength( 0 ); | ||
| expect( result.current.bandData ).toHaveLength( 0 ); | ||
| } ); | ||
|
|
||
| test( 'forecast only when forecastStart is before all data', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 10 }, | ||
| { date: new Date( '2024-01-10' ), value: 20 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors: accessorsNoBounds, | ||
| forecastStart: new Date( '2023-12-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| expect( result.current.historical ).toHaveLength( 0 ); | ||
| expect( result.current.forecast ).toHaveLength( 2 ); | ||
| } ); | ||
|
|
||
| test( 'band data requires both valid lower AND upper bounds', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 10, lower: 5, upper: 15 }, | ||
| { date: new Date( '2024-01-10' ), value: 20, lower: 15, upper: 25 }, | ||
| { date: new Date( '2024-01-15' ), value: 30, lower: null, upper: 35 }, | ||
| { date: new Date( '2024-01-20' ), value: 40, lower: 35, upper: null }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors, | ||
| forecastStart: new Date( '2024-01-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| // Only the first two points have both bounds | ||
| expect( result.current.bandData ).toHaveLength( 2 ); | ||
| expect( result.current.bandData[ 0 ] ).toEqual( { | ||
| date: new Date( '2024-01-05' ), | ||
| lower: 5, | ||
| upper: 15, | ||
| } ); | ||
| expect( result.current.bandData[ 1 ] ).toEqual( { | ||
| date: new Date( '2024-01-10' ), | ||
| lower: 15, | ||
| upper: 25, | ||
| } ); | ||
| } ); | ||
|
|
||
| test( 'points with only one bound are in forecast but excluded from bandData', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-10' ), value: 20, lower: 15, upper: null }, | ||
| { date: new Date( '2024-01-15' ), value: 30, lower: null, upper: 35 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors, | ||
| forecastStart: new Date( '2024-01-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| expect( result.current.forecast ).toHaveLength( 2 ); | ||
| expect( result.current.bandData ).toHaveLength( 0 ); | ||
| } ); | ||
|
|
||
| test( 'asNumber guard treats NaN and Infinity as null', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-10' ), value: 20, lower: NaN, upper: 25 }, | ||
| { date: new Date( '2024-01-15' ), value: 30, lower: 25, upper: Infinity }, | ||
| { date: new Date( '2024-01-20' ), value: 40, lower: -Infinity, upper: 45 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors, | ||
| forecastStart: new Date( '2024-01-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| // None have both valid lower AND upper after asNumber guard | ||
| expect( result.current.bandData ).toHaveLength( 0 ); | ||
|
|
||
| // But all are in forecast (they're still valid points) | ||
| expect( result.current.forecast ).toHaveLength( 3 ); | ||
| } ); | ||
|
|
||
| test( 'yDomain includes band bounds beyond just values', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 50, lower: 10, upper: 90 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors, | ||
| forecastStart: new Date( '2024-01-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| const [ yMin, yMax ] = result.current.yDomain; | ||
| // Lower bound (10) should pull yMin below value (50) | ||
| expect( yMin ).toBeLessThan( 10 ); | ||
| // Upper bound (90) should push yMax above value (50) | ||
| expect( yMax ).toBeGreaterThan( 90 ); | ||
| } ); | ||
|
|
||
| test( 'yDomain has minimum padding of 1 for flat data', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 50 }, | ||
| { date: new Date( '2024-01-10' ), value: 50 }, | ||
| { date: new Date( '2024-01-15' ), value: 50 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors: accessorsNoBounds, | ||
| forecastStart: new Date( '2024-02-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| const [ yMin, yMax ] = result.current.yDomain; | ||
| // Padding should be at least 1 even though range is 0 | ||
| expect( yMax - yMin ).toBeGreaterThanOrEqual( 2 ); | ||
| expect( yMin ).toBe( 49 ); | ||
| expect( yMax ).toBe( 51 ); | ||
| } ); | ||
|
|
||
| test( 'originalIndex preserves pre-sort position', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-20' ), value: 30 }, // index 0 in input | ||
| { date: new Date( '2024-01-05' ), value: 10 }, // index 1 in input | ||
| { date: new Date( '2024-01-10' ), value: 20 }, // index 2 in input | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors: accessorsNoBounds, | ||
| forecastStart: new Date( '2024-02-01' ), | ||
| } ) | ||
| ); | ||
|
|
||
| // After sorting by date: Jan 5 (idx 1), Jan 10 (idx 2), Jan 20 (idx 0) | ||
| expect( result.current.allPoints[ 0 ].originalIndex ).toBe( 1 ); | ||
| expect( result.current.allPoints[ 1 ].originalIndex ).toBe( 2 ); | ||
| expect( result.current.allPoints[ 2 ].originalIndex ).toBe( 0 ); | ||
| } ); | ||
|
|
||
| test( 'band data only comes from forecast region', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 10, lower: 5, upper: 15 }, | ||
| { date: new Date( '2024-01-15' ), value: 20, lower: 15, upper: 25 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors, | ||
| forecastStart: new Date( '2024-01-10' ), | ||
| } ) | ||
| ); | ||
|
|
||
| // First point is historical (before forecastStart), so only the second appears in bandData | ||
| expect( result.current.bandData ).toHaveLength( 1 ); | ||
| expect( result.current.bandData[ 0 ].date.getTime() ).toBe( | ||
| new Date( '2024-01-15' ).getTime() | ||
| ); | ||
| } ); | ||
|
|
||
| test( 'xDomain spans the full date range', () => { | ||
| const data: TestDatum[] = [ | ||
| { date: new Date( '2024-01-05' ), value: 10 }, | ||
| { date: new Date( '2024-01-20' ), value: 30 }, | ||
| ]; | ||
|
|
||
| const { result } = renderHook( () => | ||
| useForecastData( { | ||
| data, | ||
| accessors: accessorsNoBounds, | ||
| forecastStart: new Date( '2024-01-10' ), | ||
| } ) | ||
| ); | ||
|
|
||
| expect( result.current.xDomain[ 0 ].getTime() ).toBe( new Date( '2024-01-05' ).getTime() ); | ||
| expect( result.current.xDomain[ 1 ].getTime() ).toBe( new Date( '2024-01-20' ).getTime() ); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
The PR description claims there are "12 hook unit tests" but there are actually 13 test cases in this file. Please update the PR description to accurately reflect the test count.
3 tasks
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.
Related: CHARTS-145
Adds docs + tests for #46844
Proposed changes:
useForecastDatahook covering data transformation, sorting, domain computation, band filtering, and edge casesTimeSeriesForecastChartUnresponsivecovering rendering, legend visibility, forecast divider logic, and empty data handlingOther information:
Does this pull request change what data or activity we track or use?
No, it does not.
Testing instructions:
Storybook documentation
pnpm run storybookfromprojects/js-packages/chartsTests
pnpm run test -- "time-series-forecast-chart"fromprojects/js-packages/chartspnpm run test