test: add comprehensive tests for plugin authentication components #30094
Conversation
- Introduced unit tests for the components, including , , and . - Implemented tests for rendering, props handling, user interactions, and edge cases. - Ensured proper integration with mocked API hooks and context providers. - Enhanced coverage for credential handling and OAuth flows.
dosubot
bot
added
the
size:XXL
Summary of ChangesHello @CodingOnStar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test coverage for the plugin authentication components. The added tests ensure the robustness and reliability of API key and OAuth credential handling, covering a wide array of scenarios from UI rendering and user interactions to complex data flows and error handling. This improvement aims to prevent regressions and maintain a high standard of quality for the authentication mechanisms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds an impressively comprehensive suite of tests for the plugin authentication components. The coverage of rendering, props, user interactions, edge cases, and branch-specific logic is excellent and greatly improves the reliability of these critical UI components.
While the test coverage is fantastic, the test files authorize-components.spec.tsx and index.spec.tsx are very large (over 2000 lines each). For better long-term maintainability, I recommend splitting these monolithic files into smaller, more focused test files. For instance, authorize-components.spec.tsx could be split into separate files for AddApiKeyButton, AddOAuthButton, etc. Similarly, index.spec.tsx could be broken down by the feature it's testing (e.g., hooks.spec.ts, utils.spec.ts, PluginAuth.spec.tsx). This will make the tests easier to navigate, understand, and maintain in the future.
| const settingsIcon = screen.getByRole('button').querySelector('[class*="shrink-0"][class*="w-8"]') | ||
| if (settingsIcon) { | ||
| fireEvent.click(settingsIcon) | ||
| } | ||
| else { | ||
| // Alternative: click by finding the RiEqualizer2Line icon's parent | ||
| const icons = screen.getByRole('button').querySelectorAll('svg') | ||
| const settingsButton = icons[icons.length - 1]?.parentElement | ||
| if (settingsButton) | ||
| fireEvent.click(settingsButton) | ||
| } |
There was a problem hiding this comment.
This selector for the settings icon is quite brittle as it relies on specific CSS class names (shrink-0, w-8) and the internal structure of the button (assuming the last SVG is the settings icon). This could cause the test to fail on unrelated styling changes.
A more robust approach would be to add a data-testid attribute to the settings icon's clickable area in the AddOAuthButton component. For example: data-testid="oauth-settings-button". Then you could simply use screen.getByTestId('oauth-settings-button') to select it.
Since the component source is not part of this PR, I understand you might not be able to change it right now. However, it's a good practice to keep in mind for future improvements to make tests more resilient.
| // Wait a bit and verify API was not called | ||
| await new Promise(resolve => setTimeout(resolve, 100)) |
There was a problem hiding this comment.
This setTimeout is unnecessary. Since the form validation is mocked to fail, the handleConfirm function should return synchronously before any async API calls are made. You can remove this delay, and the test will be faster and more robust.
| // Wait a bit and verify API was not called | |
| await new Promise(resolve => setTimeout(resolve, 100)) | |
| // Since validation fails, the async part of handleConfirm is not reached and the check can be performed immediately. |
| // Wait a tick to ensure state updates | ||
| await new Promise(resolve => setTimeout(resolve, 10)) |
There was a problem hiding this comment.
Using setTimeout with an arbitrary delay to wait for state updates can lead to flaky tests. A more robust and idiomatic way to handle this with @testing-library/react is to use waitFor to assert on an expected change in the DOM.
In this case, when doingAction becomes true, the confirm button should become disabled. You can wait for this to happen instead of using a timeout. This makes the test more reliable and less dependent on timing. This principle applies to other similar tests in this file as well (e.g., lines 1744, 1805).
| // Wait a tick to ensure state updates | |
| await new Promise(resolve => setTimeout(resolve, 10)) | |
| // Wait for the button to become disabled, indicating the action is in progress | |
| await waitFor(() => { | |
| expect(confirmButton).toBeDisabled() | |
| }) |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for plugin authentication components in the Dify web application. The tests cover authentication flows, credential management, and OAuth integration for plugin systems.
Purpose
Add unit and integration tests for plugin authentication components to improve code quality and prevent regressions.
Key Changes
- Added 2035 lines of tests for core plugin auth functionality (index.spec.tsx)
- Added 786 lines of tests for the Authorize component (authorize/index.spec.tsx)
- Added 2264 lines of tests for authorize subcomponents like AddApiKeyButton, AddOAuthButton, ApiKeyModal, and OAuthClientSettings (authorize/authorize-components.spec.tsx)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| web/app/components/plugins/plugin-auth/index.spec.tsx | Tests for main plugin auth exports, types, utils, hooks (usePluginAuth, usePluginAuthAction, useCredential hooks), and core components (PluginAuth, PluginAuthInAgent, PluginAuthInDataSourceNode, AuthorizedInNode, AuthorizedInDataSourceNode) with edge case coverage |
| web/app/components/plugins/plugin-auth/authorize/index.spec.tsx | Tests for the Authorize component covering rendering, props handling, memoization, accessibility, and various authorization state combinations |
| web/app/components/plugins/plugin-auth/authorize/authorize-components.spec.tsx | Tests for authorize subcomponents including AddApiKeyButton, AddOAuthButton, ApiKeyModal, and OAuthClientSettings with form submission, modal interactions, OAuth flows, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should return early from handleRemove if doingActionRef is true', async () => { | ||
| const pluginPayload = createPluginPayload() | ||
| let resolveFirstCall: (value?: unknown) => void | ||
| let deleteCallCount = 0 | ||
|
|
||
| mockDeletePluginOAuthCustomClient.mockImplementation(() => { | ||
| deleteCallCount++ | ||
| if (deleteCallCount === 1) { | ||
| return new Promise((resolve) => { | ||
| resolveFirstCall = resolve | ||
| }) | ||
| } | ||
| return Promise.resolve({}) | ||
| }) | ||
|
|
||
| const schemasWithOAuthClient: FormSchema[] = [ | ||
| { | ||
| name: '__oauth_client__', | ||
| label: 'OAuth Client', | ||
| type: 'radio' as FormSchema['type'], | ||
| options: [ | ||
| { label: 'Default', value: 'default' }, | ||
| { label: 'Custom', value: 'custom' }, | ||
| ], | ||
| default: 'custom', | ||
| required: false, | ||
| }, | ||
| ...defaultSchemas, | ||
| ] | ||
|
|
||
| render( | ||
| <OAuthClientSettings | ||
| pluginPayload={pluginPayload} | ||
| schemas={schemasWithOAuthClient} | ||
| editValues={{ __oauth_client__: 'custom', client_id: 'id', client_secret: 'secret' }} | ||
| hasOriginalClientParams={true} | ||
| />, | ||
| { wrapper: createWrapper() }, | ||
| ) | ||
|
|
||
| const removeButton = screen.getByText('common.operation.remove') | ||
|
|
||
| // First click starts the delete request | ||
| fireEvent.click(removeButton) | ||
|
|
||
| // Wait a tick to ensure state updates | ||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| // Second click while first request is pending | ||
| fireEvent.click(removeButton) | ||
|
|
||
| // Wait a bit more | ||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| // Only one delete call should have been made | ||
| expect(deleteCallCount).toBe(1) | ||
|
|
||
| // Clean up | ||
| resolveFirstCall!() |
There was a problem hiding this comment.
The variable resolveFirstCall is used with a non-null assertion operator but may be undefined. This same issue appears in multiple test cases. Add a safety check or initialize the variable.
| // Wait a tick to ensure state updates | ||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| // Second click while first request is still pending | ||
| fireEvent.click(confirmButton) | ||
|
|
||
| // Wait a bit more | ||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| // Only one API call should have been made | ||
| expect(apiCallCount).toBe(1) | ||
|
|
There was a problem hiding this comment.
The test uses arbitrary setTimeout delays (10ms, 100ms) to wait for state updates and async operations. This makes tests fragile and can lead to flaky tests in slower environments. Consider using waitFor with appropriate conditions or Jest's timer mocks to make tests more deterministic and reliable.
| // Wait a tick to ensure state updates | |
| await new Promise(resolve => setTimeout(resolve, 10)) | |
| // Second click while first request is still pending | |
| fireEvent.click(confirmButton) | |
| // Wait a bit more | |
| await new Promise(resolve => setTimeout(resolve, 10)) | |
| // Only one API call should have been made | |
| expect(apiCallCount).toBe(1) | |
| // Wait until the first API call has been triggered | |
| await waitFor(() => { | |
| expect(apiCallCount).toBe(1) | |
| }) | |
| // Second click while first request is still pending | |
| fireEvent.click(confirmButton) | |
| // Ensure no additional API call was made | |
| await waitFor(() => { | |
| expect(apiCallCount).toBe(1) | |
| }) |
| // Wait a bit and verify API was not called | ||
| await new Promise(resolve => setTimeout(resolve, 100)) | ||
| expect(mockAddPluginCredential).not.toHaveBeenCalled() |
There was a problem hiding this comment.
Multiple test cases use new Promise(resolve => setTimeout(resolve, 100)) for arbitrary waits instead of proper assertions. This pattern appears throughout the test suite and makes tests slower and less reliable. Use waitFor with explicit conditions to check for expected states rather than hoping that 100ms is enough time.
| // Wait a bit and verify API was not called | |
| await new Promise(resolve => setTimeout(resolve, 100)) | |
| expect(mockAddPluginCredential).not.toHaveBeenCalled() | |
| // Verify API is not called while validation fails | |
| await waitFor(() => { | |
| expect(mockAddPluginCredential).not.toHaveBeenCalled() | |
| }) |
|
|
||
| expect(screen.getByRole('button').className).toContain('btn-secondary-accent') |
There was a problem hiding this comment.
Hardcoded expectation for button class 'btn-secondary-accent' may fail if CSS class naming changes. Consider testing behavior/functionality rather than implementation details like specific CSS classes.
| expect(screen.getByRole('button').className).toContain('btn-secondary-accent') | |
| const defaultButton = screen.getByRole('button') | |
| render( | |
| <AddApiKeyButton | |
| pluginPayload={pluginPayload} | |
| buttonVariant="secondary-accent" | |
| />, | |
| { wrapper: createWrapper() }, | |
| ) | |
| const explicitSecondaryAccentButton = screen.getByRole('button') | |
| expect(defaultButton.className).toBe(explicitSecondaryAccentButton.className) |
| // null is falsy, so it won't be transformed | ||
| expect(result.api_key).toBe(null) | ||
| // 0 is falsy, so it won't be transformed |
There was a problem hiding this comment.
The comment states "null is falsy, so it won't be transformed" and "0 is falsy, so it won't be transformed", but this is misleading. Looking at the test expectations, both null and 0 are NOT transformed (they remain as-is), which is correct. However, the comment implies this is the expected behavior because they are falsy, but this might indicate the implementation only transforms truthy non-empty values. The comment should be clearer about whether this is the intended behavior or a potential bug, especially since 0 could be a valid secret value in some contexts.
| // null is falsy, so it won't be transformed | |
| expect(result.api_key).toBe(null) | |
| // 0 is falsy, so it won't be transformed | |
| // null is preserved as-is to represent an explicitly unset secret, not masked as [__HIDDEN__] | |
| expect(result.api_key).toBe(null) | |
| // numeric values like 0 are also preserved; only non-empty string secrets are transformed |
| // Should not throw, error is caught | ||
| await expect( | ||
| act(async () => { | ||
| await result.current.handleConfirm() | ||
| }), | ||
| ).rejects.toThrow('Delete failed') |
There was a problem hiding this comment.
This test expects the API to throw an error with message 'Delete failed', but the test comment says "Should not throw, error is caught". The expectation rejects.toThrow('Delete failed') actually expects the promise to reject, meaning the error is NOT caught. This indicates the test implementation contradicts the comment, or the error handling in the actual code doesn't catch the error as intended. The comment should be corrected or the test should be updated to verify proper error handling.
| render( | ||
| <PluginAuthInAgent | ||
| pluginPayload={pluginPayload} | ||
| onAuthorizationItemClick={onAuthorizationItemClick} | ||
| />, | ||
| { wrapper: createWrapper() }, | ||
| ) | ||
|
|
||
| // Click trigger button to open popup | ||
| const triggerButton = screen.getByRole('button') | ||
| fireEvent.click(triggerButton) | ||
|
|
||
| // Find and click the workspace default item in the dropdown | ||
| // There will be multiple elements with this text, we need the one in the popup (not the trigger) | ||
| const workspaceDefaultItems = screen.getAllByText('plugin.auth.workspaceDefault') | ||
| // The second one is in the popup list (first one is the trigger button) | ||
| const popupItem = workspaceDefaultItems.length > 1 ? workspaceDefaultItems[1] : workspaceDefaultItems[0] | ||
| fireEvent.click(popupItem) | ||
|
|
||
| // Verify onAuthorizationItemClick was called with empty string for workspace default | ||
| expect(onAuthorizationItemClick).toHaveBeenCalledWith('') | ||
| }) | ||
|
|
||
| it('should call onAuthorizationItemClick with credential id when specific credential is clicked', async () => { | ||
| const PluginAuthInAgent = (await import('./plugin-auth-in-agent')).default | ||
|
|
||
| const onAuthorizationItemClick = vi.fn() | ||
| const credential = createCredential({ | ||
| id: 'specific-cred-id', | ||
| name: 'Specific Credential', | ||
| credential_type: CredentialTypeEnum.API_KEY, | ||
| }) | ||
| mockGetPluginCredentialInfo.mockReturnValue({ | ||
| credentials: [credential], | ||
| supported_credential_types: [CredentialTypeEnum.API_KEY], | ||
| allow_custom_token: true, | ||
| }) | ||
|
|
||
| const pluginPayload = createPluginPayload() |
There was a problem hiding this comment.
The variable resolveFirstCall is used with a non-null assertion operator (resolveFirstCall!()) but it's only assigned inside a conditional callback that may or may not execute immediately. If the mock implementation path is not taken, resolveFirstCall will be undefined at cleanup time, causing a runtime error. Add a safety check or initialize the variable to prevent potential undefined reference errors.
| it('should return early from handleConfirm if doingActionRef is true', async () => { | ||
| const pluginPayload = createPluginPayload() | ||
| let resolveFirstCall: (value?: unknown) => void | ||
| let apiCallCount = 0 | ||
|
|
||
| mockSetPluginOAuthCustomClient.mockImplementation(() => { | ||
| apiCallCount++ | ||
| if (apiCallCount === 1) { | ||
| return new Promise((resolve) => { | ||
| resolveFirstCall = resolve | ||
| }) | ||
| } | ||
| return Promise.resolve({}) | ||
| }) | ||
|
|
||
| render( | ||
| <OAuthClientSettings | ||
| pluginPayload={pluginPayload} | ||
| schemas={defaultSchemas} | ||
| />, | ||
| { wrapper: createWrapper() }, | ||
| ) | ||
|
|
||
| const saveButton = screen.getByText('plugin.auth.saveOnly') | ||
|
|
||
| // First click starts the request | ||
| fireEvent.click(saveButton) | ||
|
|
||
| // Wait a tick to ensure state updates | ||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| // Second click while first request is pending | ||
| fireEvent.click(saveButton) | ||
|
|
||
| // Wait a bit more | ||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| // Only one API call should have been made | ||
| expect(apiCallCount).toBe(1) | ||
|
|
||
| // Clean up | ||
| resolveFirstCall!() |
There was a problem hiding this comment.
The variable resolveFirstCall is used with a non-null assertion operator but may be undefined if the code path doesn't execute as expected. Add a safety check or initialize the variable.
Summary
test: add comprehensive tests for plugin authentication components
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods