codeant-ai-for-open-source[bot] commented on code in PR #38033:
URL: https://github.com/apache/superset/pull/38033#discussion_r3564749808
##########
superset-frontend/src/pages/Register/Register.test.tsx:
##########
@@ -17,32 +17,63 @@
* under the License.
*/
import { render, screen } from 'spec/helpers/testing-library';
-import { MemoryRouter } from 'react-router-dom';
+import { MemoryRouter, Route } from 'react-router-dom';
+import getBootstrapData from 'src/utils/getBootstrapData';
import Register from './index';
jest.mock('src/utils/getBootstrapData', () => ({
__esModule: true,
- default: () => ({
+ default: jest.fn(() => ({
common: {
conf: {
RECAPTCHA_PUBLIC_KEY: '',
},
},
- }),
+ })),
+}));
+
+const mockMakeUrl = jest.fn((path: string) => path);
+jest.mock('src/utils/pathUtils', () => ({
+ makeUrl: (...args: string[]) => mockMakeUrl(...args),
}));
Review Comment:
**Suggestion:** The module mock is stubbing `makeUrl`, but the Register page
now resolves its login link via `ensureAppRoot` from `navigationUtils`. Because
`ensureAppRoot` is not provided by this mock, activation-page renders can crash
with a function-not-found error when building the login href. Mock
`ensureAppRoot` (or mock `navigationUtils` directly) instead of only `makeUrl`.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Register activation tests crash with TypeError on login render.
⚠️ SUPERSET_APP_ROOT login link behavior lacks reliable coverage.
⚠️ navigationUtils-pathUtils contract obscured by misleading test mocks.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/pages/Register/Register.test.tsx:35-38`, the
test defines
`mockMakeUrl` and calls `jest.mock('src/utils/pathUtils', () => ({ makeUrl:
(...args:
string[]) => mockMakeUrl(...args), }))`, replacing the entire `pathUtils`
module with an
object that only exports `makeUrl`.
2. In `superset-frontend/src/utils/navigationUtils.ts:24-38`,
`navigationUtils` imports `{
ensureAppRoot, makeUrl, stripAppRoot }` from `'./pathUtils'` and re-exports
them (`export
{ ensureAppRoot, makeUrl, stripAppRoot };`). Because the Jest mock provides
only
`makeUrl`, the imported `ensureAppRoot` and `stripAppRoot` become
`undefined` in the test
environment.
3. In `superset-frontend/src/pages/Register/index.tsx:35,79-105`, the
`Register` component
imports `ensureAppRoot` from `'src/utils/navigationUtils'` and, in the
activation-success
branch (`if (activationHash)`), renders a `Button` with
`href={ensureAppRoot('/login/')}`
and `data-test="login-button"`.
4. When the test calls `renderActivated()` (`Register.test.tsx:66-73`),
React renders the
activation-success page and executes `ensureAppRoot('/login/')`. Because
`ensureAppRoot`
is `undefined` due to the incorrect `pathUtils` mock, this call throws a
`TypeError:
ensureAppRoot is not a function`, causing the activation-page tests to crash
instead of
validating the login link or SUPERSET_APP_ROOT behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2e914652bf64daca0fb008f578f1d80&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d2e914652bf64daca0fb008f578f1d80&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Register/Register.test.tsx
**Line:** 35:38
**Comment:**
*Api Mismatch: The module mock is stubbing `makeUrl`, but the Register
page now resolves its login link via `ensureAppRoot` from `navigationUtils`.
Because `ensureAppRoot` is not provided by this mock, activation-page renders
can crash with a function-not-found error when building the login href. Mock
`ensureAppRoot` (or mock `navigationUtils` directly) instead of only `makeUrl`.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38033&comment_hash=2af7a3682953f9ccb3b62e548387fd066addfa7df54aefe0fe78eabbba8c812f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38033&comment_hash=2af7a3682953f9ccb3b62e548387fd066addfa7df54aefe0fe78eabbba8c812f&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/Register/Register.test.tsx:
##########
@@ -80,3 +111,71 @@ test('should render input placeholders', () => {
expect(screen.getByPlaceholderText('Password')).toBeInTheDocument();
expect(screen.getByPlaceholderText('Confirm password')).toBeInTheDocument();
});
+
+// --- Recaptcha tests ---
+
+test('should not render captcha when RECAPTCHA_PUBLIC_KEY is empty', () => {
+ renderRegister();
+ expect(screen.queryByTestId('captcha-input')).not.toBeInTheDocument();
+});
+
+test('should render captcha when RECAPTCHA_PUBLIC_KEY is set', () => {
+ mockGetBootstrapData.mockReturnValue({
+ common: {
+ conf: {
+ RECAPTCHA_PUBLIC_KEY: 'test-key-123',
+ },
+ },
+ } as ReturnType<typeof getBootstrapData>);
+ renderRegister();
+ expect(screen.getByTestId('captcha-input')).toBeInTheDocument();
+});
+
+// --- Activation success page tests ---
+
+test('should render activation success page when activationHash is present',
() => {
+ renderActivated();
+ expect(screen.getByText('Registration successful')).toBeInTheDocument();
+ expect(
+ screen.getByText(
+ 'Your account is activated. You can log in with your credentials.',
+ ),
+ ).toBeInTheDocument();
+ expect(screen.getByTestId('login-button')).toBeInTheDocument();
+});
+
+test('should not render registration form on activation page', () => {
+ renderActivated();
+ expect(screen.queryByTestId('username-input')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('register-button')).not.toBeInTheDocument();
+});
+
+test('should call makeUrl for login link on activation page', () => {
+ renderActivated();
+ expect(mockMakeUrl).toHaveBeenCalledWith('/login/');
+});
Review Comment:
**Suggestion:** This assertion is checking calls to `makeUrl`, but the
component no longer uses `makeUrl` for the activation-page login link, so this
test will fail even when behavior is correct. Update the assertion to track the
helper actually used by the component (`ensureAppRoot`) and verify the rendered
href accordingly. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Activation login-link test asserts obsolete makeUrl helper usage.
⚠️ Correct ensureAppRoot-based behavior may still fail this test.
⚠️ Developers may chase false negatives during registration flow changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the Register component at
`superset-frontend/src/pages/Register/index.tsx:35,79-105`: when
`activationHash` is
present, it renders a success `Result` whose `extra` array includes a
`Button` with
`href={ensureAppRoot('/login/')}` and `data-test="login-button"`. There is
no reference to
`makeUrl` anywhere in this component.
2. In `superset-frontend/src/utils/pathUtils.ts:53-69,85-87`,
`ensureAppRoot(path)` is the
primary helper that prefixes router-relative paths with the application root;
`makeUrl(path)` is just a thin wrapper that returns `ensureAppRoot(path)`.
The Register
page accesses `ensureAppRoot` via `navigationUtils`
(`navigationUtils.ts:24,38`), not
`makeUrl` directly.
3. The test `should call makeUrl for login link on activation page` at
`Register.test.tsx:153-156` calls `renderActivated()` (defined at lines
66-73) and then
asserts `expect(mockMakeUrl).toHaveBeenCalledWith('/login/');`. Given the
component’s
current implementation, the login link is built through
`ensureAppRoot('/login/')` and
never calls `makeUrl`, so `mockMakeUrl` is not a reliable proxy for the
actual helper the
component uses.
4. As a result, once the module mocking is corrected to provide a working
`ensureAppRoot`
(for example, by mocking `navigationUtils` or exporting `ensureAppRoot` from
the
`pathUtils` mock), this test will either fail despite correct behavior
(because `makeUrl`
is not invoked) or force an artificial implementation where `ensureAppRoot`
calls
`makeUrl` just to satisfy the assertion, meaning the test is validating an
obsolete
contract instead of the real `ensureAppRoot`-based login-link behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90c823d5326d4c09b26a979b49e4b58b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=90c823d5326d4c09b26a979b49e4b58b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Register/Register.test.tsx
**Line:** 153:156
**Comment:**
*Logic Error: This assertion is checking calls to `makeUrl`, but the
component no longer uses `makeUrl` for the activation-page login link, so this
test will fail even when behavior is correct. Update the assertion to track the
helper actually used by the component (`ensureAppRoot`) and verify the rendered
href accordingly.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38033&comment_hash=918742635d7fe9ee5002eec701d98b5c26cd78488f960217c110fa2e620b1e20&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38033&comment_hash=918742635d7fe9ee5002eec701d98b5c26cd78488f960217c110fa2e620b1e20&reaction=dislike'>👎</a>
--
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]
