codeant-ai-for-open-source[bot] commented on PR #34479: URL: https://github.com/apache/superset/pull/34479#issuecomment-3638879242
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-93f82a2792e7e008dc91d724a914188589026397e4264d9efa0124a748931c0cR93-R100'><strong>Route Overwrite Missing</strong></a><br>A dynamic route handler is registered inside the test using `fetchMock.get(...)` while a static route for the same endpoint was already registered in `beforeEach`. Without specifying route overwrite or using a one-time matcher, the earlier route may match first and the dynamic handler (which resolves the Promise) will never be invoked — causing the test to hang or be flaky.<br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-9b1cae83b79b684488185352975d4442b7231ef4ef5734b3b0379eae3f91fbb3R27-R95'><strong>Test isolation</strong></a><br>`fetchMock` is used as a shared/global instance in multiple describe blocks. If tests run in parallel (or other suites also use the global fetch-mock), there is potential for cross-test interference. Consider scoping a fresh sandboxed instance per test to guarantee isolation.<br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-9b1cae83b79b684488185352975d4442b7231ef4ef5734b3b0379eae3f91fbb3R30-R36'><strong>Route overwrite behavior</strong></a><br>The PR removed the previous use of the `overwriteRoutes: true` option when registering the GET route and instead relies on registering the same route each beforeEach + calling `fetchMock.hardReset()` in afterEach. Confirm the new approach preserves the former semantics: ensure tests don't accidentally accumulate duplicate routes (or unexpected route ordering) across runs, and validate that the route registered in beforeEach always takes effect even if a previous route remained.<br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-9b1cae83b79b684488185352975d4442b7231ef4ef5734b3b0379eae3f91fbb3R34-R36'><strong>Reset vs restore semantics</strong></a><br>The code now uses `fetchMock.hardReset()` in afterEach. Verify this resets both the registered routes and the global `fetch` (or other behavior expected previously by `restore()`), and that it matches test-suite expectations. If `hardReset()` does not restore the original global fetch, other tests might be affected.<br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-db1c48716dbf89f66730e54877de39a08104febb2011cfb4a67b15a93c19766aR36-R51'><strong>Type assertion may mask missing props</strong></a><br>`createProps` ends with a cast `as PropertiesModalProps`. Casting the literal can hide required/mismatched prop types (the test will still compile but runtime behavior may be incorrect if the component expects other props). Ensure `PropertiesModalProps` actually includes `addSuccessToast` and any other required fields, or return a properly typed object instead of asserting.<br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-8e847e05bd54f09132c8ca8f9f5c4456c8d2b755074a0192f8395fd77cbfafbfR44-R50'><strong>Test isolation</strong></a><br>The test suite resets fetch-mock before and after tests, but it does not restore the original global `fetch`. This can leak mocked fetch into other tests in the suite. Ensure the original fetch is restored after tests or use a sandboxed instance so other tests remain unaffected.<br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-8e847e05bd54f09132c8ca8f9f5c4456c8d2b755074a0192f8395fd77cbfafbfR45-R49'><strong>API compatibility</strong></a><br>The test now calls `fetchMock.hardReset()`. Confirm that `hardReset` exists and has the intended semantics in the bumped fetch-mock v12.5.3 API (it may behave differently or be removed). If it doesn't exist or behaves differently, tests will fail or not properly reset mock state.<br> - [ ] <a href='https://github.com/apache/superset/pull/34479/files#diff-93f82a2792e7e008dc91d724a914188589026397e4264d9efa0124a748931c0cR69-R76'><strong>Initial mock route precedence</strong></a><br>The `beforeEach` registers `fetchMock.get(DATASOURCE_ENDPOINT, [])` each test. If later in the same test you intend to override that route, you must either register the initial route with `{ overwriteRoutes: true }` when appropriate or explicitly restore/reset before re-registering. Otherwise ordering causes the earlier registration to take precedence.<br> </td></tr> </table> -- 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]
