Skip to content

test #174: created unit test for Avatar component #637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jefische wants to merge 9 commits into
base: develop
Choose a base branch
from
+53 −20
Open

test #174: created unit test for Avatar component #637

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a13cac1
test: #174 initial unit test for Avatar component
jefische Oct 14, 2025
b3f2662
adding test scenario and alt text for AvatarImage
jefische Oct 16, 2025
2b12edc
Merge branch 'develop' into jeremy/174-unit-test-avatar-component
jefische Oct 20, 2025
d87c46f
Revise tests for Avatar component after review
jefische Oct 22, 2025
d7931ee
Merge branch 'develop' into jeremy/174-unit-test-avatar-component
jefische Oct 22, 2025
009fece
Merge branch 'develop' into jeremy/174-unit-test-avatar-component
jefische Oct 24, 2025
faafeea
Merge branch 'develop' into jeremy/174-unit-test-avatar-component
jefische Oct 30, 2025
82ab88f
Merge branch 'develop' into jeremy/174-unit-test-avatar-component
jefische Nov 5, 2025
bf87659
address avatar mocking issues for ProfileCard component
jefische Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions components/Avatar/Avatar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import Avatar from './Avatar';
import { render, screen } from '@testing-library/react';

jest.mock('@radix-ui/react-avatar', () => {
Copy link
Member

@nickytonline nickytonline Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI for anyone coming across this. My first instinct was to ask why are we mocking this as you always want to mock as little as possible or not at all so we are testing the actual implementation.

In this case, we mock Radix’s Avatar.Image because in JSDOM, what we use in Jest tests, image load events don’t fire as it's not a real browser environment, so only the fallback is ever shown in tests if we don't mock.

const actual = jest.requireActual('@radix-ui/react-avatar');
return ({
...actual,
Image: ({src, alt, ...props}: {src?: string, alt?: string}) => {
Copy link
Contributor

@suzuhe481 suzuhe481 Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove ...props from the Image mock. When testing, you should only mock the necessary behavior. For the Avatar component, you're currently only testing for src and alt. The other properties of Image aren't needed.

Copy link
Contributor

@suzuhe481 suzuhe481 Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the optional ? on the alt?. This property will always be included in both the AvatarImage and AvatarFallback.

return <img data-testid="avatar-image" src={src} alt={alt} {...props}/>;
Copy link
Contributor

@suzuhe481 suzuhe481 Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add data-testids inside components used in testing. While you fetched the element by role, which is good, the id still isn't necessary for the mock.

If anything, this id should have been added to the AvatarImage component.

}
})
});

describe('Avatar Component', () => {

it('Renders the component with user image using valid src attribute', () => {
Copy link
Collaborator

@shashilo shashilo Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the fallback component show here?

Copy link
Contributor Author

@jefische jefische Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this scenario. I wanted to test that the user provided image is correctly added to the document.

Copy link
Collaborator

@shashilo shashilo Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an exception to check that fallback doesn't show in this default scenario.

Copy link
Contributor Author

@jefische jefische Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly the fallback image is still showing in the document for this scenario. I tried using .not.toBeVisible() method as well as .not.toBeInTheDocument() but both tests fail:

1st attempt:

+ const avatarElementFallbackImage = screen.getByTestId('avatar-fallback')
+ expect(avatarElementFallbackImage).not.toBeVisible();

2nd attempt:

+ const avatarElementFallbackImage = screen.getByTestId('avatar-fallback')
+ expect(avatarElementFallbackImage).not.toBeInTheDocument();

I tried looking at screen.debug() and it's showing both elements in the JSDOM. Are both of these supposed to show up like this?

Screen-debug-avatar-component

render(<Avatar userAvatar='https://mdbcdn.b-cdn.net/img/new/avatars/2.webp' />);

const avatarElementWithImage = screen.getByRole('img', { name: 'user avatar'});
Copy link
Collaborator

@shashilo shashilo Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow how we're doing the other components by adding a data-testid attribute.

expect(avatarElementWithImage).toBeInTheDocument();
expect(avatarElementWithImage).toHaveAttribute(
'src',
'https://mdbcdn.b-cdn.net/img/new/avatars/2.webp',
);

});

it('Renders the fallback image when src is undefined', () => {
Copy link
Collaborator

@shashilo shashilo Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when there's a string given bug the image is not found?

Copy link
Contributor Author

@jefische jefische Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I'll add this test scenario.

render(<Avatar userAvatar={undefined} />);

const avatarElementFallbackImage = screen.getByRole('img', { name: 'default avatar'});
expect(avatarElementFallbackImage).toBeInTheDocument();
});

});
2 changes: 1 addition & 1 deletion components/Avatar/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const Avatar = ({
}): JSX.Element => {
return (
<AvatarBody>
<AvatarImage src={userAvatar} alt="" />
<AvatarImage src={userAvatar} alt="user avatar" />
Copy link
Member

@nickytonline nickytonline Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactor (a11y): "user avatar" is too generic. If the avatar conveys meaning, add more context, like the username, otherwise leave the alt test to "".

Suggested change
<AvatarImage src={userAvatar} alt="user avatar" />
<AvatarImage src={userAvatar} alt=`profile image for user ${username}` />

Copy link
Contributor Author

@jefische jefische Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding more context with the username makes sense. After adding username as a prop:

const Avatar = ({ userAvatar, username }: { userAvatar: string | undefined, username: string })

I believe I would need to update each instance where the Avatar component is used:

elecretanta/components/UserDropdownMenu/UserDropdownMenu.tsx

Line 43 in 0a2d352

<Avatar userAvatar={avatar} />

elecretanta/components/ProfileCard/ProfileCard.tsx

Line 92 in 0a2d352

<Avatar data-testid="avatar" userAvatar={profile?.avatar} />

elecretanta/components/MembersList/MembersList.tsx

Line 31 in 0a2d352

<Avatar userAvatar={member.member.avatar} />

I want to make sure this correct and okay to do inside the same PR as I'm starting to change multiple files. For the UserDropdownMenu component it gets more complicated to pass a username as well.

Copy link
Collaborator

@shashilo shashilo Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is only a unit test task. This should be removed into an A11Y ticket. I want to keep things simple for your first test.

Copy link
Contributor Author

@jefische jefische Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll just leave the AvatarImage alt text as it was prior to my edits.

<AvatarFallback>
<img
src="https://static.vecteezy.com/system/resources/previews/024/183/525/non_2x/avatar-of-a-man-portrait-of-a-young-guy-illustration-of-male-character-in-modern-color-style-vector.jpg"
Expand Down
Loading