codeant-ai-for-open-source[bot] commented on code in PR #36270: URL: https://github.com/apache/superset/pull/36270#discussion_r2664398183
########## superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/dateFilterComparator.test.ts: ########## @@ -0,0 +1,126 @@ +/** + * 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 dateFilterComparator from '../../src/utils/dateFilterComparator'; + +test('returns 0 when filter date equals cell date', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 local + const cellDate = new Date('2003-10-08T12:00:00Z'); // Oct 8, 2003 UTC + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); +}); + +test('returns -1 when cell date is before filter date', () => { + const filterDate = new Date(2003, 9, 10); // Oct 10, 2003 + const cellDate = new Date('2003-10-08T12:00:00Z'); // Oct 8, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('returns 1 when cell date is after filter date', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 + const cellDate = new Date('2003-10-10T12:00:00Z'); // Oct 10, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(1); +}); + +test('returns -1 when cell value is null', () => { + const filterDate = new Date(2003, 9, 8); + + expect(dateFilterComparator(filterDate, null as unknown as Date)).toBe(-1); +}); + +test('returns -1 when cell value is undefined', () => { + const filterDate = new Date(2003, 9, 8); + + expect(dateFilterComparator(filterDate, undefined as unknown as Date)).toBe( + -1, + ); +}); + +test('returns -1 when cell value is an invalid date string', () => { + const filterDate = new Date(2003, 9, 8); + const cellDate = new Date('invalid-date'); + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('handles year boundary - cell in previous year', () => { + const filterDate = new Date(2024, 0, 1); // Jan 1, 2024 + const cellDate = new Date('2023-12-31T12:00:00Z'); // Dec 31, 2023 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('handles year boundary - cell in next year', () => { + const filterDate = new Date(2023, 11, 31); // Dec 31, 2023 + const cellDate = new Date('2024-01-01T12:00:00Z'); // Jan 1, 2024 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(1); +}); + +test('handles month boundary - cell in previous month', () => { + const filterDate = new Date(2003, 9, 1); // Oct 1, 2003 + const cellDate = new Date('2003-09-30T12:00:00Z'); // Sep 30, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('handles month boundary - cell in next month', () => { + const filterDate = new Date(2003, 8, 30); // Sep 30, 2003 + const cellDate = new Date('2003-10-01T12:00:00Z'); // Oct 1, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(1); +}); + +test('matches UTC midnight timestamp', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 + const cellDate = new Date('2003-10-08T00:00:00Z'); // Oct 8, 2003 00:00 UTC + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); +}); + +test('matches UTC end-of-day timestamp', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 + const cellDate = new Date('2003-10-08T23:59:59Z'); // Oct 8, 2003 23:59 UTC + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); +}); + +test('correctly compares dates from ISO string cell values', () => { + const filterDate = new Date(2003, 9, 8); + const cellDate = new Date('2003-10-08'); Review Comment: **Suggestion:** Ambiguous ISO date parsing: the string "2003-10-08" can be parsed differently across JS engines (UTC vs local), making this test flaky; use an explicit UTC timestamp string so the parsed Date is deterministic. [possible bug] **Severity Level:** Critical 🚨 ```suggestion const cellDate = new Date('2003-10-08T00:00:00Z'); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The 'YYYY-MM-DD' format can be parsed differently across JS engines (local vs UTC), making the test flaky. Replacing it with an explicit UTC timestamp ('2003-10-08T00:00:00Z') makes parsing deterministic and aligns with other tests that assert UTC behaviour. This improves test reliability and directly addresses a plausible flakiness. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/dateFilterComparator.test.ts **Line:** 108:108 **Comment:** *Possible Bug: Ambiguous ISO date parsing: the string "2003-10-08" can be parsed differently across JS engines (UTC vs local), making this test flaky; use an explicit UTC timestamp string so the parsed Date is deterministic. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/dateFilterComparator.test.ts: ########## @@ -0,0 +1,126 @@ +/** + * 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 dateFilterComparator from '../../src/utils/dateFilterComparator'; + +test('returns 0 when filter date equals cell date', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 local + const cellDate = new Date('2003-10-08T12:00:00Z'); // Oct 8, 2003 UTC + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); +}); + +test('returns -1 when cell date is before filter date', () => { + const filterDate = new Date(2003, 9, 10); // Oct 10, 2003 + const cellDate = new Date('2003-10-08T12:00:00Z'); // Oct 8, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('returns 1 when cell date is after filter date', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 + const cellDate = new Date('2003-10-10T12:00:00Z'); // Oct 10, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(1); +}); + +test('returns -1 when cell value is null', () => { + const filterDate = new Date(2003, 9, 8); + + expect(dateFilterComparator(filterDate, null as unknown as Date)).toBe(-1); +}); + +test('returns -1 when cell value is undefined', () => { + const filterDate = new Date(2003, 9, 8); + + expect(dateFilterComparator(filterDate, undefined as unknown as Date)).toBe( + -1, + ); +}); + +test('returns -1 when cell value is an invalid date string', () => { + const filterDate = new Date(2003, 9, 8); + const cellDate = new Date('invalid-date'); + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('handles year boundary - cell in previous year', () => { + const filterDate = new Date(2024, 0, 1); // Jan 1, 2024 + const cellDate = new Date('2023-12-31T12:00:00Z'); // Dec 31, 2023 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('handles year boundary - cell in next year', () => { + const filterDate = new Date(2023, 11, 31); // Dec 31, 2023 + const cellDate = new Date('2024-01-01T12:00:00Z'); // Jan 1, 2024 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(1); +}); + +test('handles month boundary - cell in previous month', () => { + const filterDate = new Date(2003, 9, 1); // Oct 1, 2003 + const cellDate = new Date('2003-09-30T12:00:00Z'); // Sep 30, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(-1); +}); + +test('handles month boundary - cell in next month', () => { + const filterDate = new Date(2003, 8, 30); // Sep 30, 2003 + const cellDate = new Date('2003-10-01T12:00:00Z'); // Oct 1, 2003 + + expect(dateFilterComparator(filterDate, cellDate)).toBe(1); +}); + +test('matches UTC midnight timestamp', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 + const cellDate = new Date('2003-10-08T00:00:00Z'); // Oct 8, 2003 00:00 UTC + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); +}); + +test('matches UTC end-of-day timestamp', () => { + const filterDate = new Date(2003, 9, 8); // Oct 8, 2003 + const cellDate = new Date('2003-10-08T23:59:59Z'); // Oct 8, 2003 23:59 UTC + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); +}); + +test('correctly compares dates from ISO string cell values', () => { + const filterDate = new Date(2003, 9, 8); + const cellDate = new Date('2003-10-08'); + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); +}); + +test('handles cell value passed as timestamp number', () => { + const filterDate = new Date(2003, 9, 8); + // Oct 8, 2003 12:00:00 UTC as timestamp + const cellDate = new Date(Date.UTC(2003, 9, 8, 12, 0, 0)); + + expect(dateFilterComparator(filterDate, cellDate)).toBe(0); Review Comment: **Suggestion:** Test name says it passes a timestamp number but the test actually passes a Date object; this doesn't exercise the numeric-timestamp code path. Pass the raw timestamp number (Date.UTC(...)) to the comparator so the test verifies handling of numeric cell values. [logic error] **Severity Level:** Minor ⚠️ ```suggestion const cellTimestamp = Date.UTC(2003, 9, 8, 12, 0, 0); expect(dateFilterComparator(filterDate, cellTimestamp as unknown as Date)).toBe(0); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The test name says it's exercising the numeric-timestamp path but it passes a Date object instead. The comparator constructs a new Date(cellValue) and will behave slightly differently for a raw number input. Changing the test to pass the numeric timestamp (with a TS cast) ensures the intended code path is covered and increases test coverage. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/dateFilterComparator.test.ts **Line:** 116:118 **Comment:** *Logic Error: Test name says it passes a timestamp number but the test actually passes a Date object; this doesn't exercise the numeric-timestamp code path. Pass the raw timestamp number (Date.UTC(...)) to the comparator so the test verifies handling of numeric cell values. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- 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]
