Copilot commented on code in PR #36315: URL: https://github.com/apache/superset/pull/36315#discussion_r2572410980
########## superset-frontend/plugins/legacy-plugin-chart-calendar/test/cal-heatmap.test.ts: ########## @@ -0,0 +1,45 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import CalendarChartPlugin from '../src/index'; +import CalHeatMap from '../src/vendor/cal-heatmap'; + +/** + * The example tests in this file act as a starting point, and + * we encourage you to build more. + */ +describe('@superset-ui/legacy-plugin-chart-calendar', () => { + it('exists', () => { + expect(CalendarChartPlugin).toBeDefined(); + }); +}); + +/** + * This test tests that a reasonble starting date is selected given range + * and starting date. Since time offset is updated to be not considered, + * it would only be 1-1-2025 + */ +describe('getMonthDomain filter month-day', () => { +it('returns correct month domain for 2025-01-01 to 2025-02-31', () => { Review Comment: Inconsistent indentation: The `it` statement should be indented by 2 spaces to align with the standard indentation pattern in the test file. ```suggestion it('returns correct month domain for 2025-01-01 to 2025-02-31', () => { ``` ########## superset-frontend/plugins/legacy-plugin-chart-calendar/test/cal-heatmap.test.ts: ########## @@ -0,0 +1,45 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import CalendarChartPlugin from '../src/index'; +import CalHeatMap from '../src/vendor/cal-heatmap'; + +/** + * The example tests in this file act as a starting point, and + * we encourage you to build more. + */ +describe('@superset-ui/legacy-plugin-chart-calendar', () => { + it('exists', () => { + expect(CalendarChartPlugin).toBeDefined(); + }); +}); + +/** + * This test tests that a reasonble starting date is selected given range + * and starting date. Since time offset is updated to be not considered, + * it would only be 1-1-2025 + */ +describe('getMonthDomain filter month-day', () => { +it('returns correct month domain for 2025-01-01 to 2025-02-31', () => { + const getMonthDomain = (CalHeatMap as any).prototype.getMonthDomain; + const d = new Date(Date.UTC(2025, 0, 1)); + const range = 2; + const result = getMonthDomain(d, range); + expect(result[0].toISOString()).toContain('2025-01-01'); + }); Review Comment: Insufficient test coverage: The test only covers one scenario (normal positive range). To properly validate the fix for issue #21870, the test should include: 1. The specific edge case from the issue (end date on first day of month) 2. Backward ranges (if supported) 3. Date objects as range parameters (as supported by the function signature) 4. Multi-year ranges to ensure UTC handling works correctly across year boundaries ########## superset/viz.py: ########## @@ -799,6 +799,10 @@ def get_data(self, df: pd.DataFrame) -> VizData: # pylint: disable=too-many-loc if domain == "year": range_ = end.year - start.year + 1 elif domain == "month": + # extra day to exclude first day of the month + end = end - rdelta.relativedelta(days=1) + diff_delta = rdelta.relativedelta(end, start) + diff_secs = (end - start).total_seconds() Review Comment: Unused variable: `diff_secs` is recalculated after modifying `end` on line 805, but it's not used anywhere in the `month` domain case. This recalculation appears to be unnecessary and should be removed. ```suggestion ``` ########## superset/viz.py: ########## @@ -799,6 +799,10 @@ def get_data(self, df: pd.DataFrame) -> VizData: # pylint: disable=too-many-loc if domain == "year": range_ = end.year - start.year + 1 elif domain == "month": + # extra day to exclude first day of the month + end = end - rdelta.relativedelta(days=1) + diff_delta = rdelta.relativedelta(end, start) + diff_secs = (end - start).total_seconds() range_ = diff_delta.years * 12 + diff_delta.months + 1 Review Comment: Missing test coverage: The backend changes to the `CalHeatmapViz.get_data` method's month domain calculation logic lack corresponding Python tests. Other viz classes in the repository have tests in `tests/integration_tests/viz_tests.py`. Consider adding tests that verify: 1. The correct range calculation when the end date is the first day of a month 2. Edge cases like same-month ranges, multi-year ranges 3. Verification that the fix resolves issue #21870 ########## superset-frontend/plugins/legacy-plugin-chart-calendar/test/cal-heatmap.test.ts: ########## @@ -0,0 +1,45 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import CalendarChartPlugin from '../src/index'; +import CalHeatMap from '../src/vendor/cal-heatmap'; + +/** + * The example tests in this file act as a starting point, and + * we encourage you to build more. + */ +describe('@superset-ui/legacy-plugin-chart-calendar', () => { + it('exists', () => { + expect(CalendarChartPlugin).toBeDefined(); + }); +}); + +/** + * This test tests that a reasonble starting date is selected given range + * and starting date. Since time offset is updated to be not considered, + * it would only be 1-1-2025 + */ +describe('getMonthDomain filter month-day', () => { +it('returns correct month domain for 2025-01-01 to 2025-02-31', () => { + const getMonthDomain = (CalHeatMap as any).prototype.getMonthDomain; + const d = new Date(Date.UTC(2025, 0, 1)); + const range = 2; + const result = getMonthDomain(d, range); + expect(result[0].toISOString()).toContain('2025-01-01'); Review Comment: The test assertion is incomplete. It only checks that the first element contains '2025-01-01', but doesn't verify the length of the returned array or check that the correct number of months (2) are returned, which is the actual behavior being tested according to the comment. ########## superset-frontend/plugins/legacy-plugin-chart-calendar/test/cal-heatmap.test.ts: ########## @@ -0,0 +1,45 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import CalendarChartPlugin from '../src/index'; +import CalHeatMap from '../src/vendor/cal-heatmap'; + +/** + * The example tests in this file act as a starting point, and + * we encourage you to build more. + */ +describe('@superset-ui/legacy-plugin-chart-calendar', () => { + it('exists', () => { + expect(CalendarChartPlugin).toBeDefined(); + }); +}); + +/** + * This test tests that a reasonble starting date is selected given range Review Comment: Spelling error: "reasonble" should be "reasonable". ```suggestion * This test tests that a reasonable starting date is selected given range ``` ########## superset-frontend/plugins/legacy-plugin-chart-calendar/test/cal-heatmap.test.ts: ########## @@ -0,0 +1,45 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import CalendarChartPlugin from '../src/index'; +import CalHeatMap from '../src/vendor/cal-heatmap'; + +/** + * The example tests in this file act as a starting point, and + * we encourage you to build more. + */ +describe('@superset-ui/legacy-plugin-chart-calendar', () => { + it('exists', () => { + expect(CalendarChartPlugin).toBeDefined(); + }); +}); + +/** + * This test tests that a reasonble starting date is selected given range + * and starting date. Since time offset is updated to be not considered, + * it would only be 1-1-2025 + */ +describe('getMonthDomain filter month-day', () => { +it('returns correct month domain for 2025-01-01 to 2025-02-31', () => { Review Comment: The test description mentions "2025-02-31" which is an invalid date (February only has 28 or 29 days). This should likely be "2025-03-01" to match the test setup using `range = 2` (2 months from 2025-01-01). ```suggestion it('returns correct month domain for 2025-01-01 to 2025-03-01', () => { ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
