Add tests for skeleton partial components #89
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds five new unit test files under tests/partials for skeleton Vue components, verifying presence of root elements, animate-pulse class defaults, aria-hidden semantics, optional isAnimated disabling, and wrapperClass merging where applicable. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gocanto, 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 suite by adding dedicated Vitest unit tests for various skeleton partial components. The new tests cover critical aspects such as animation toggling, structural rendering, and accessibility attributes, ensuring the robustness and correct behavior of these loading state indicators across the application. This improves the overall quality and maintainability of the UI components. Highlights
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 introduces Vitest test coverage for several skeleton partial components. The tests are a great addition and correctly verify the animation toggling. My review provides feedback to improve test quality and enhance accessibility coverage. I've pointed out a redundant assertion, a test with a logical contradiction, and opportunities to add assertions for the aria-hidden attribute to ensure these decorative components are not exposed to assistive technologies.
| it('is marked as purely decorative', () => { | ||
| const wrapper = mount(WidgetSkillsSkeletonPartial); | ||
|
|
||
| expect(wrapper.attributes('aria-hidden')).toBeUndefined(); | ||
| expect(wrapper.classes()).toContain('animate-pulse'); | ||
| }); |
There was a problem hiding this comment.
This test's description, 'is marked as purely decorative', contradicts its assertion. A decorative element should be hidden from assistive technologies with aria-hidden="true". However, the test asserts toBeUndefined(), which means it's exposed to screen readers.
For consistency with other skeleton components and for better accessibility, this component should have aria-hidden="true". This test should be updated to enforce this, which will require a corresponding fix in the WidgetSkillsSkeletonPartial.vue component.
| it('is marked as purely decorative', () => { | |
| const wrapper = mount(WidgetSkillsSkeletonPartial); | |
| expect(wrapper.attributes('aria-hidden')).toBeUndefined(); | |
| expect(wrapper.classes()).toContain('animate-pulse'); | |
| }); | |
| it('is marked as purely decorative', () => { | |
| const wrapper = mount(WidgetSkillsSkeletonPartial); | |
| expect(wrapper.attributes('aria-hidden')).toBe('true'); | |
| expect(wrapper.classes()).toContain('animate-pulse'); | |
| }); |
| it('is animated by default', () => { | ||
| const wrapper = mount(ArticleItemSkeletonPartial); | ||
|
|
||
| expect(wrapper.classes()).toContain('animate-pulse'); |
There was a problem hiding this comment.
It's good practice to also assert that skeleton components are hidden from assistive technologies. The component correctly has aria-hidden="true", so please add a check for it in the test.
expect(wrapper.classes()).toContain('animate-pulse');
expect(wrapper.attributes('aria-hidden')).toBe('true');| const wrapper = mount(PostPageSkeletonPartial); | ||
|
|
||
| const root = wrapper.get('[data-testid="post-page-skeleton"]'); | ||
| expect(root.exists()).toBe(true); |
There was a problem hiding this comment.
The wrapper.get() method throws an error if the element is not found, making this expect assertion redundant. The test's purpose is fulfilled by get() succeeding. You can remove this line for a more concise test.
|
|
||
| expect(wrapper.classes()).not.toContain('animate-pulse'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test suite is missing a check for accessibility. Skeleton loaders are purely decorative and should be hidden from assistive technologies using aria-hidden="true". Please add a test case to ensure this attribute is present. Note that this will require updating the ProjectCardSkeletonPartial.vue component to include aria-hidden="true" on its root element to make the test pass.
| }); | |
| }); | |
| it('is hidden from assistive technology', () => { | |
| const wrapper = mount(ProjectCardSkeletonPartial); | |
| expect(wrapper.attributes('aria-hidden')).toBe('true'); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/partials/ProjectCardSkeletonPartial.test.ts (1)
1-4: Optional: standardize selectors with data-testidConsider adding data-testid="project-card-skeleton" on the component wrapper and targeting it in tests for stability.
tests/partials/TalkCardSkeletonPartial.test.ts (1)
1-4: Operational: ensure Vitest v3.2.x and alias configPin vitest to latest 3.2.x and verify @partials alias is resolved in vitest config to avoid env-only failures. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/partials/ArticleItemSkeletonPartial.test.ts(1 hunks)tests/partials/PostPageSkeletonPartial.test.ts(1 hunks)tests/partials/ProjectCardSkeletonPartial.test.ts(1 hunks)tests/partials/TalkCardSkeletonPartial.test.ts(1 hunks)tests/partials/WidgetSkillsSkeletonPartial.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Unit Tests
tests/partials/ArticleItemSkeletonPartial.test.ts
[error] 9-9: Assertion failed: ArticleItemSkeletonPartial is animated by default but does not include 'animate-pulse' class.
tests/partials/TalkCardSkeletonPartial.test.ts
[error] 9-9: Assertion failed: TalkCardSkeletonPartial is animated by default but does not include 'animate-pulse' class.
tests/partials/ProjectCardSkeletonPartial.test.ts
[error] 9-9: Assertion failed: ProjectCardSkeletonPartial is animated by default but does not include 'animate-pulse' class.
🪛 GitHub Check: vitest
tests/partials/ArticleItemSkeletonPartial.test.ts
[failure] 9-9: tests/partials/ArticleItemSkeletonPartial.test.ts > ArticleItemSkeletonPartial > is animated by default
AssertionError: expected [ 'py-5', 'border-b', …(2) ] to include 'animate-pulse'
❯ tests/partials/ArticleItemSkeletonPartial.test.ts:9:29
tests/partials/TalkCardSkeletonPartial.test.ts
[failure] 9-9: tests/partials/TalkCardSkeletonPartial.test.ts > TalkCardSkeletonPartial > applies animation by default
AssertionError: expected [ 'relative', 'aspect-video', …(13) ] to include 'animate-pulse'
❯ tests/partials/TalkCardSkeletonPartial.test.ts:9:29
tests/partials/ProjectCardSkeletonPartial.test.ts
[failure] 9-9: tests/partials/ProjectCardSkeletonPartial.test.ts > ProjectCardSkeletonPartial > renders animated wrapper by default
AssertionError: expected [ 'rounded-lg', 'border', …(6) ] to include 'animate-pulse'
❯ tests/partials/ProjectCardSkeletonPartial.test.ts:9:29
🔇 Additional comments (3)
tests/partials/WidgetSkillsSkeletonPartial.test.ts (1)
5-22: LGTMTests are clear and focused; assertions on item count and classes are good.
tests/partials/PostPageSkeletonPartial.test.ts (1)
5-19: LGTMStable test id + accessibility and animation checks look good.
tests/partials/ProjectCardSkeletonPartial.test.ts (1)
6-10: Retainwrapper.classes()assertions —animate-pulseis bound on the root element.Likely an incorrect or invalid review comment.
| it('is animated by default', () => { | ||
| const wrapper = mount(ArticleItemSkeletonPartial); | ||
|
|
||
| expect(wrapper.classes()).toContain('animate-pulse'); | ||
| }); |
There was a problem hiding this comment.
Make animation checks resilient to non-root placement
Search for a descendant with the animation class; ensure disabled case asserts none exist.
Apply this diff:
@@
it('is animated by default', () => {
const wrapper = mount(ArticleItemSkeletonPartial);
- expect(wrapper.classes()).toContain('animate-pulse');
+ expect(wrapper.find('.animate-pulse').exists()).toBe(true);
});
@@
it('can disable the animation', () => {
const wrapper = mount(ArticleItemSkeletonPartial, {
props: { isAnimated: false },
});
- expect(wrapper.classes()).not.toContain('animate-pulse');
+ expect(wrapper.find('.animate-pulse').exists()).toBe(false);
});Also applies to: 12-18
🧰 Tools
🪛 GitHub Actions: Unit Tests
[error] 9-9: Assertion failed: ArticleItemSkeletonPartial is animated by default but does not include 'animate-pulse' class.
🪛 GitHub Check: vitest
[failure] 9-9: tests/partials/ArticleItemSkeletonPartial.test.ts > ArticleItemSkeletonPartial > is animated by default
AssertionError: expected [ 'py-5', 'border-b', …(2) ] to include 'animate-pulse'
❯ tests/partials/ArticleItemSkeletonPartial.test.ts:9:29
🤖 Prompt for AI Agents
In tests/partials/ArticleItemSkeletonPartial.test.ts around lines 6-10 (and
similarly update lines 12-18), the tests assume the animation class is on the
root element; change them to search for a descendant with the animation class
instead and update the disabled-case assertion to assert no descendant exists:
use the wrapper.find or wrapper.findAll API to look for '.animate-pulse' and
assert .exists() is true for the default/animated test and false (or length ===
0) for the disabled test so the checks remain resilient to non-root placement.
| it('applies animation by default', () => { | ||
| const wrapper = mount(TalkCardSkeletonPartial); | ||
|
|
||
| expect(wrapper.classes()).toContain('animate-pulse'); | ||
| expect(wrapper.attributes('aria-hidden')).toBe('true'); | ||
| }); |
There was a problem hiding this comment.
Fix failing animation assertions to target descendant
Component likely doesn’t set animate-pulse on root. Search for a descendant instead; ensure disabled asserts none exist.
Apply this diff:
@@
it('applies animation by default', () => {
const wrapper = mount(TalkCardSkeletonPartial);
- expect(wrapper.classes()).toContain('animate-pulse');
+ expect(wrapper.find('.animate-pulse').exists()).toBe(true);
expect(wrapper.attributes('aria-hidden')).toBe('true');
});
@@
it('respects disabled animation flag', () => {
const wrapper = mount(TalkCardSkeletonPartial, {
props: { isAnimated: false },
});
- expect(wrapper.classes()).not.toContain('animate-pulse');
+ expect(wrapper.find('.animate-pulse').exists()).toBe(false);
});Also applies to: 13-19
🧰 Tools
🪛 GitHub Actions: Unit Tests
[error] 9-9: Assertion failed: TalkCardSkeletonPartial is animated by default but does not include 'animate-pulse' class.
🪛 GitHub Check: vitest
[failure] 9-9: tests/partials/TalkCardSkeletonPartial.test.ts > TalkCardSkeletonPartial > applies animation by default
AssertionError: expected [ 'relative', 'aspect-video', …(13) ] to include 'animate-pulse'
❯ tests/partials/TalkCardSkeletonPartial.test.ts:9:29
🤖 Prompt for AI Agents
In tests/partials/TalkCardSkeletonPartial.test.ts around lines 6-11 and 13-19,
the assertions are checking the root element for the 'animate-pulse' class but
the component applies that class to a descendant; update the test to query the
descendant element (e.g., using wrapper.find or wrapper.findComponent with the
selector/class for the animated element) and assert that the descendant
hasClass('animate-pulse') for the default case, and for the disabled case assert
that no matching descendant exists or that it does not have the 'animate-pulse'
class.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68df7c594b988333a027318dff37dbf1
Summary by CodeRabbit