pawarprasad123 commented on code in PR #688:
URL: https://github.com/apache/atlas/pull/688#discussion_r3840511503
##########
dashboard/src/views/SideBar/__tests__/SideBarBody.test.tsx:
##########
@@ -497,11 +500,32 @@ describe('SideBarBody', () => {
expect(mockHandleOpenAboutModal).toHaveBeenCalled();
});
- it('should pass loading prop to ClassificationTree', () => {
- const props = { ...defaultProps, loading: true };
- renderWithProviders(props);
-
- expect(screen.getByTestId('classification-tree')).toBeInTheDocument();
+ it('should show "Version unavailable" if versionError is set', () => {
+ const stateWithVersionError = {
+ session: {
+ versionData: {
+ loading: false,
+ data: null,
+ error: { message: "Failed to fetch version" }
+ }
+ }
+ };
+ renderWithProviders({}, { store: createMockStore(stateWithVersionError)
});
+
+ expect(screen.getByText('Version unavailable')).toBeInTheDocument();
+ });
+
+ it('should hide relationships icon when relationshipSearch is falsy', ()
=> {
Review Comment:
1) line 518-531
This test does not validate the behavior. SideBarBody reads
relationshipSearch from globalSessionData in @utils/Enum, which is mocked as {
relationshipSearch: true } at line 143–152. The Redux store override has no
effect. Also, r_relationshipTreeRender is a data-cy attribute, not data-testid.
Fix: Override the Enum mock for this test, collapse the drawer, and assert
queryByAltText('relationships') and queryByTestId('relationships-tree') are
absent.
2)
Only Entities and Custom Filters active states are tested. Please add cases
for:
Glossary (?gtype=... or /glossary/...)
Classification (?tag=...)
Business Metadata (/administrator/businessMetadata)
Relationships (?relationshipName=...)
##########
dashboard/src/components/EntityDisplayImage.tsx:
##########
@@ -38,7 +39,7 @@ const DisplayImage = ({
const primaryUrl = getEntityIconPath({ entityData }) || "";
const fallbackUrl = getEntityIconPath({ entityData, errorUrl: primaryUrl })
|| "";
- const handleError = (e: React.SyntheticEvent<HTMLImageElement, Event>) => {
+ const handleError = (e: SyntheticEvent<HTMLImageElement, Event>) => {
Review Comment:
line 42-47, verify and than fix
target.src is absolute in the browser; fallbackUrl may be relative, so the
comparison can be unreliable. Prefer !target.src.endsWith(fallbackUrl) or
compare target.getAttribute('src').
##########
dashboard/src/views/SideBar/SideBarTree/SideBarTree.tsx:
##########
@@ -970,10 +970,7 @@ const BarTreeView: FC<{
<LightTooltip title={label} disableHoverListener={!isOverflown}>
<span
Review Comment:
searchTerm in the useEffect dependency array triggers unnecessary overflow
recalculations and triggers an ESLint warning. [label] alone should be
sufficient since overflow depends on rendered text, not the search filter state
directly.
##########
dashboard/src/redux/slice/__tests__/sessionSlice.test.ts:
##########
@@ -150,5 +162,61 @@ describe('sessionSlice', () => {
expect(globalSession).toHaveBeenCalledWith(mockData);
});
+
+ describe('fetchVersionData', () => {
+ it('should handle fetchVersionData.pending', () => {
+ const action = { type: fetchVersionData.pending.type };
+ const state = sessionReducer(undefined, action);
+
+ expect(state.versionData.loading).toBe(true);
+ expect(state.versionData.data).toBeNull();
+ expect(state.versionData.error).toBeNull();
+ });
+
+ it('should handle fetchVersionData.fulfilled', () => {
+ const mockVersionData = { Version: '3.0.0' };
+
+ const action = {
+ type: fetchVersionData.fulfilled.type,
+ payload: mockVersionData
+ };
+ const state = sessionReducer(undefined, action);
+
+ expect(state.versionData.loading).toBe(false);
+ expect(state.versionData.data).toEqual(mockVersionData);
+ expect(state.versionData.error).toBeNull();
+ });
+
+ it('should handle fetchVersionData.rejected', () => {
+ const error = 'Error fetching version data';
+ const action = {
+ type: fetchVersionData.rejected.type,
+ payload: error
+ };
+ const state = sessionReducer(undefined, action);
+
+ expect(state.versionData.loading).toBe(false);
+ expect(state.versionData.data).toBeNull();
+ expect(state.versionData.error).toBe(error);
+ });
+
+ it('should fetch version data successfully', async () => {
+ const { getVersion } =
require('../../../api/apiMethods/headerApiMethods');
+ const mockVersionData = { Version: '3.0.0' };
+ getVersion.mockResolvedValue({ data: mockVersionData });
+
+ const store = configureStore({
+ reducer: {
+ session: sessionReducer
+ }
+ });
+
+ await store.dispatch(fetchVersionData());
+
+ const state = store.getState().session;
+ expect(state.versionData.loading).toBe(false);
+ expect(state.versionData.data).toEqual(mockVersionData);
+ });
Review Comment:
Add an async integration test for fetchVersionData rejection (similar to
fetchSessionData error test at line 131).
##########
dashboard/src/views/Layout/__tests__/About.test.tsx:
##########
@@ -169,253 +143,55 @@ describe('About', () => {
expect(listItem).toHaveAttribute('data-target', '_blank')
})
- it('should render empty version when API returns empty data object',
async () => {
- mockGetVersion.mockResolvedValue({
- data: {}
- })
+ it('should render empty version when data object is empty', () => {
+ useAppSelectorSpy.mockReturnValue({ data: {}, loading: false })
render(<About />)
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
// Version should be displayed but empty
expect(screen.getByText(/Version:/i)).toBeInTheDocument()
const versionTypography = screen.getByTestId('typography-body1')
expect(versionTypography).toBeInTheDocument()
expect(versionTypography.textContent).toContain('Version:')
})
- it('should render empty version when API returns undefined data', async
() => {
- mockGetVersion.mockResolvedValue({
- data: undefined
- })
+ it('should render empty version when data is undefined', () => {
+ useAppSelectorSpy.mockReturnValue({ data: undefined, loading:
false })
render(<About />)
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
expect(screen.getByText(/Version:/i)).toBeInTheDocument()
})
- it('should render empty version when API returns null data', async ()
=> {
- mockGetVersion.mockResolvedValue({
- data: null
- })
+ it('should render empty version when data is null', () => {
+ useAppSelectorSpy.mockReturnValue({ data: null, loading: false
})
render(<About />)
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
expect(screen.getByText(/Version:/i)).toBeInTheDocument()
})
- it('should handle API call error and call serverError', async () => {
- const mockError = new Error('Network error')
- mockGetVersion.mockRejectedValue(mockError)
+ it('should handle versionData with undefined Version property', () => {
+ useAppSelectorSpy.mockReturnValue({ data: { Description: 'Some
description' }, loading: false })
render(<About />)
- await waitFor(() => {
- expect(mockServerError).toHaveBeenCalledWith(mockError,
expect.any(Object))
- })
-
- // Should still show content (not skeleton) after error
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
- expect(screen.getByText(/Version:/i)).toBeInTheDocument()
- })
-
- it('should handle API call error with response data', async () => {
- const mockError = {
- response: {
- data: {
- errorMessage: 'Server error occurred'
- }
- }
- }
- mockGetVersion.mockRejectedValue(mockError)
-
- render(<About />)
-
- await waitFor(() => {
- expect(mockServerError).toHaveBeenCalledWith(mockError,
expect.any(Object))
- })
-
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
- })
-
- it('should call getVersion on component mount', async () => {
- mockGetVersion.mockResolvedValue({
- data: { Version: '1.0.0' }
- })
-
- render(<About />)
-
- expect(mockGetVersion).toHaveBeenCalledTimes(1)
- expect(mockGetVersion).toHaveBeenCalledWith()
-
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
- })
-
- it('should render correct Typography variants and colors', async () => {
- mockGetVersion.mockResolvedValue({
- data: { Version: '2.0.0' }
- })
-
- render(<About />)
-
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
- // Check Typography variants
- const body1Typography = screen.getByTestId('typography-body1')
- expect(body1Typography).toBeInTheDocument()
-
- const body2Typographies =
screen.getAllByTestId('typography-body2')
- expect(body2Typographies.length).toBeGreaterThan(0)
-
- // Check color prop for "Get involved!" text
- const getInvolvedTypography = body2Typographies.find(
- (el) => el.textContent === 'Get involved!'
- )
- expect(getInvolvedTypography).toHaveAttribute('data-color',
'info.main')
- })
-
- it('should render Stack components with correct props', async () => {
- mockGetVersion.mockResolvedValue({
- data: { Version: '1.0.0' }
- })
-
- render(<About />)
-
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
- // Check main Stack
- const mainStack = screen.getByTestId('stack')
- expect(mainStack).toHaveAttribute('data-spacing', '2')
-
- // Check column Stack
- const columnStack = screen.getByTestId('stack-column')
- expect(columnStack).toHaveAttribute('data-spacing', '1')
- expect(columnStack).toHaveAttribute('data-direction', 'column')
- })
-
- it('should render List with dense prop', async () => {
- mockGetVersion.mockResolvedValue({
- data: { Version: '1.0.0' }
- })
-
- render(<About />)
-
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
- const list = screen.getByTestId('list')
- expect(list).toHaveAttribute('data-dense', 'true')
- })
-
- it('should render ListItemText with correct primary text', async () => {
- mockGetVersion.mockResolvedValue({
- data: { Version: '1.0.0' }
- })
-
- render(<About />)
-
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
- const listItemText = screen.getByTestId('list-item-text')
- expect(listItemText).toHaveAttribute(
- 'data-primary',
- 'Licensed under the Apache License Version 2.0'
- )
- })
-
- it('should handle versionData with undefined Version property', async
() => {
- mockGetVersion.mockResolvedValue({
- data: { Description: 'Some description' }
- })
-
- render(<About />)
-
- await waitFor(() => {
-
expect(screen.queryByTestId('skeleton-loader')).not.toBeInTheDocument()
- })
-
expect(screen.getByText(/Version:/i)).toBeInTheDocument()
const versionTypography = screen.getByTestId('typography-body1')
expect(versionTypography.textContent).toContain('Version:')
expect(versionTypography.textContent).not.toContain('undefined')
})
- it('should set loader to false after successful API call', async () => {
- mockGetVersion.mockResolvedValue({
- data: { Version: '1.0.0' }
- })
+ it('should render gracefully on Redux error state', () => {
Review Comment:
line 185-196
Error state test should assert the exact message: "Unknown (failed to fetch
version)" (see About.tsx:40). Currently it only checks the component doesn't
crash.
--
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]
