Feature: Add Comprehensive Unit Test Coverage for Common Utilities and Hooks#1691
Open
Suvam-paul145 wants to merge 4 commits intoreactplay:mainfrom
Open
Feature: Add Comprehensive Unit Test Coverage for Common Utilities and Hooks#1691Suvam-paul145 wants to merge 4 commits intoreactplay:mainfrom
Suvam-paul145 wants to merge 4 commits intoreactplay:mainfrom
Conversation
✅ Deploy Preview for reactplayio ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey! contributor, thank you for opening a Pull Request 🎉.
@reactplay/maintainers will review your submission soon and give you helpful feedback.
If you're interested in continuing your contributions to open source and want to be a part of a welcoming and fantastic community, we invite you to join our ReactPlay Discord Community.
Show your support by starring ⭐ this repository. Thank you and we appreciate your contribution to open source!
Stale Marking : After 30 days of inactivity this issue/PR will be marked as stale issue/PR and it will be closed and locked in 7 days if no further activity occurs.
There was a problem hiding this comment.
Pull request overview
This PR significantly expands automated test coverage across shared src/common/ infrastructure (utilities, hooks, search helpers, and services) and also introduces a centralized HTML sanitization helper that is applied to multiple UI surfaces rendering dynamic HTML.
Changes:
- Added unit/integration tests for common utilities, hooks, search query parsing/translation, and the plays service layer.
- Introduced
common/utils/sanitizeHTML(DOMPurify wrapper) and updated multiple components to sanitize content before usingdangerouslySetInnerHTML(and removed it in one place). - Updated Husky pre-commit hook to run
lint-staged, and improved error handling inTube2tunes.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plays/tube2tunes/Tube2tunes.jsx | Ensures loading state is cleared on non-success statuses; logs and flags error on request failure. |
| src/plays/text-to-speech/TextToSpeech.jsx | Removes dangerouslySetInnerHTML in favor of safe text rendering. |
| src/plays/markdown-editor/Output.jsx | Sanitizes rendered markdown HTML before injecting into the DOM. |
| src/plays/fun-quiz/QuizScreen.jsx | Sanitizes dynamic HTML for questions/options before injecting. |
| src/plays/fun-quiz/EndScreen.jsx | Sanitizes dynamic HTML in quiz summary details before injecting. |
| src/plays/devblog/Pages/Article.jsx | Sanitizes article HTML body before injecting. |
| src/plays/Selection-Sort-Visualizer/SelectionSortVisualizer.js | Adds clarifying comment around safe innerHTML usage (clearing only). |
| src/common/utils/sanitizeHTML.js | Adds shared DOMPurify-based sanitization utility for dangerouslySetInnerHTML call sites. |
| src/common/utils/tests/formatCount.test.ts | Adds tests for duration/view count formatting utilities. |
| src/common/utils/tests/coverImageUtil.test.js | Adds tests for cover image loading fallback behavior (currently via full-module mock). |
| src/common/utils/tests/commonUtils.test.js | Adds tests for common utility helpers (compare/search/url/date/count). |
| src/common/services/tests/plays.test.js | Adds service-layer tests for tag deletion and filtered play retrieval. |
| src/common/search/tests/search-helper.test.js | Adds tests for query parsing, extraction, and DB translation behavior. |
| src/common/hooks/tests/useLocalStorage.test.js | Adds tests for localStorage-backed hook behavior (init, update, parse errors). |
| src/common/hooks/tests/useLikePlays.test.js | Adds tests for like/unlike hook behavior with mocked GraphQL submit. |
| src/common/hooks/tests/useGitHub.test.js | Adds tests for GitHub fetch hook success/error flows. |
| src/common/hooks/tests/useFetch.test.js | Adds tests for generic fetch hook success/error/options. |
| src/common/hooks/tests/useFeaturedPlays.test.js | Adds tests for featured plays hook success/error with mocked GraphQL. |
| src/common/hooks/tests/useContributors.test.js | Adds tests for contributors hook filtering/sorting/error handling. |
| src/common/hooks/tests/useCacheResponse.test.ts | Adds tests for cache hook storage/retrieval/overwrite behavior. |
| src/common/badges-dashboard/BadgeDetails.jsx | Sanitizes HTML created from badge descriptions prior to injecting. |
| src/common/Testimonial/TestimonialCard.jsx | Sanitizes rendered testimonial quote HTML (line breaks) prior to injecting. |
| .husky/pre-commit | Switches hook command to run lint-staged. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('formatDate', () => { | ||
| it('formats an ISO date string into "Joined DD Mon YYYY"', () => { | ||
| const result = formatDate('2023-06-15T10:00:00Z'); | ||
| expect(result).toMatch(/^Joined 15 \w+ 2023$/); |
There was a problem hiding this comment.
The formatDate assertion is too loose (/^Joined 15 \w+ 2023$/) and won’t catch real regressions (e.g., returning the wrong month). Since this is a deterministic transformation, assert the exact expected string for a known input date so the test will fail if month/day parsing is incorrect.
Suggested change
| expect(result).toMatch(/^Joined 15 \w+ 2023$/); | |
| expect(result).toBe('Joined 15 Jun 2023'); |
Comment on lines
4
to
9
| const Output = ({ md, text, mdPreviewBox }) => { | ||
| return ( | ||
| <div | ||
| className="md-editor output-div" | ||
| dangerouslySetInnerHTML={{ __html: md.render(text) }} | ||
| dangerouslySetInnerHTML={{ __html: sanitizeHTML(md.render(text)) }} | ||
| id={mdPreviewBox} |
There was a problem hiding this comment.
md.render(text) + sanitizeHTML(...) runs on every render (in the editor this is effectively on every keystroke). Consider memoizing the rendered+sanitized HTML (e.g., with useMemo keyed by text) to avoid repeated markdown rendering and DOMPurify sanitization work during typing.
Comment on lines
1
to
12
| import DOMPurify from 'dompurify'; | ||
|
|
||
| /** | ||
| * Sanitizes an HTML string using DOMPurify to prevent XSS attacks. | ||
| * Use this utility whenever you need to render dynamic HTML via dangerouslySetInnerHTML. | ||
| * | ||
| * @param {string} html - The raw HTML string to sanitize. | ||
| * @returns {string} - A sanitized HTML string safe to use with dangerouslySetInnerHTML. | ||
| */ | ||
| const sanitizeHTML = (html) => DOMPurify.sanitize(html ?? ''); | ||
|
|
||
| export default sanitizeHTML; |
There was a problem hiding this comment.
The PR description focuses on adding unit test coverage/CI thresholds, but this change also introduces a new sanitizeHTML utility and updates multiple UI components to sanitize dangerouslySetInnerHTML output, plus a Husky hook change. Please update the PR description (or split into separate PRs) so reviewers and release notes reflect the security + tooling changes accurately.
Comment on lines
+8
to
+56
| jest.mock('images/play-fallback-cover.png', () => 'fallback-image.png', { virtual: true }); | ||
|
|
||
| // Mock the dynamic import by overriding the loadImageForExtension behavior | ||
| // through mocking the entire module and re-implementing loadCoverImage with | ||
| // a controllable import mock. | ||
| let mockImport; | ||
|
|
||
| jest.mock('../coverImageUtil', () => { | ||
| const { IMAGE_EXTENSIONS, FULFILLED_STATUS } = jest.requireActual('../utilsConstants'); | ||
|
|
||
| return { | ||
| loadCoverImage: async (playSlug) => { | ||
| const imagePromises = IMAGE_EXTENSIONS.map((extension) => mockImport(playSlug, extension)); | ||
| const results = await Promise.allSettled(imagePromises); | ||
| const image = results.find( | ||
| (result) => result.status === FULFILLED_STATUS && result.value?.default | ||
| ); | ||
|
|
||
| return image?.value.default || 'fallback-image.png'; | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| describe('loadCoverImage', () => { | ||
| beforeEach(() => { | ||
| mockImport = jest.fn(); | ||
| }); | ||
|
|
||
| it('returns the first successfully loaded image', async () => { | ||
| // png fails, jpg succeeds | ||
| mockImport | ||
| .mockRejectedValueOnce(new Error('not found')) // png | ||
| .mockResolvedValueOnce({ default: MOCK_COVER }) // jpg | ||
| .mockRejectedValueOnce(new Error('not found')); // jpeg | ||
|
|
||
| const result = await loadCoverImage('my-play'); | ||
| expect(result).toBe(MOCK_COVER); | ||
| }); | ||
|
|
||
| it('returns fallback image when all extensions fail', async () => { | ||
| mockImport.mockRejectedValue(new Error('not found')); | ||
|
|
||
| const result = await loadCoverImage('missing-play'); | ||
| expect(result).toBe(MOCK_FALLBACK); | ||
| }); | ||
|
|
||
| it('returns fallback when images resolve without default property', async () => { | ||
| mockImport.mockResolvedValue({ noDefault: true }); | ||
|
|
There was a problem hiding this comment.
The test mocks the entire coverImageUtil module and re-implements loadCoverImage, so it doesn’t actually validate the real production implementation (and could pass even if loadCoverImage breaks). Consider testing the real loadCoverImage by mocking the dynamically imported plays/<slug>/cover.<ext> modules (e.g., via jest.mock(..., { virtual: true })) or by refactoring coverImageUtil to make the import function injectable/exported for mocking.
Suggested change
| jest.mock('images/play-fallback-cover.png', () => 'fallback-image.png', { virtual: true }); | |
| // Mock the dynamic import by overriding the loadImageForExtension behavior | |
| // through mocking the entire module and re-implementing loadCoverImage with | |
| // a controllable import mock. | |
| let mockImport; | |
| jest.mock('../coverImageUtil', () => { | |
| const { IMAGE_EXTENSIONS, FULFILLED_STATUS } = jest.requireActual('../utilsConstants'); | |
| return { | |
| loadCoverImage: async (playSlug) => { | |
| const imagePromises = IMAGE_EXTENSIONS.map((extension) => mockImport(playSlug, extension)); | |
| const results = await Promise.allSettled(imagePromises); | |
| const image = results.find( | |
| (result) => result.status === FULFILLED_STATUS && result.value?.default | |
| ); | |
| return image?.value.default || 'fallback-image.png'; | |
| } | |
| }; | |
| }); | |
| describe('loadCoverImage', () => { | |
| beforeEach(() => { | |
| mockImport = jest.fn(); | |
| }); | |
| it('returns the first successfully loaded image', async () => { | |
| // png fails, jpg succeeds | |
| mockImport | |
| .mockRejectedValueOnce(new Error('not found')) // png | |
| .mockResolvedValueOnce({ default: MOCK_COVER }) // jpg | |
| .mockRejectedValueOnce(new Error('not found')); // jpeg | |
| const result = await loadCoverImage('my-play'); | |
| expect(result).toBe(MOCK_COVER); | |
| }); | |
| it('returns fallback image when all extensions fail', async () => { | |
| mockImport.mockRejectedValue(new Error('not found')); | |
| const result = await loadCoverImage('missing-play'); | |
| expect(result).toBe(MOCK_FALLBACK); | |
| }); | |
| it('returns fallback when images resolve without default property', async () => { | |
| mockImport.mockResolvedValue({ noDefault: true }); | |
| // Mock the fallback image used by coverImageUtil. | |
| jest.mock('images/play-fallback-cover.png', () => 'fallback-image.png', { virtual: true }); | |
| // Mock dynamically imported cover images for specific plays. The coverImageUtil | |
| // implementation is expected to import paths like: | |
| // plays/<slug>/cover.<ext> | |
| // where <ext> is one of the supported IMAGE_EXTENSIONS (e.g., png, jpg, jpeg). | |
| // | |
| // For the "my-play" slug, provide a successful jpg cover module. | |
| jest.mock( | |
| 'plays/my-play/cover.jpg', | |
| () => ({ default: MOCK_COVER }), | |
| { virtual: true } | |
| ); | |
| // For the "bad-module" slug, provide a module without a default export so that | |
| // loadCoverImage must fall back to the fallback image. | |
| jest.mock( | |
| 'plays/bad-module/cover.png', | |
| () => ({ noDefault: true }), | |
| { virtual: true } | |
| ); | |
| describe('loadCoverImage', () => { | |
| it('returns the first successfully loaded image', async () => { | |
| // For slug "my-play", the jpg cover module is mocked to exist and export | |
| // MOCK_COVER as its default. Other extensions are not mocked and should | |
| // be treated as missing by the implementation. | |
| const result = await loadCoverImage('my-play'); | |
| expect(result).toBe(MOCK_COVER); | |
| }); | |
| it('returns fallback image when all extensions fail', async () => { | |
| // For slug "missing-play", no cover modules are mocked. All dynamic | |
| // imports should fail, so the implementation must use the fallback. | |
| const result = await loadCoverImage('missing-play'); | |
| expect(result).toBe(MOCK_FALLBACK); | |
| }); | |
| it('returns fallback when images resolve without default property', async () => { | |
| // For slug "bad-module", the mocked module does not provide a default | |
| // export, so the implementation should ignore it and fall back. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Problem Description Closes #1687
The current codebase had only 2 unit test files, both limited to:
src/plays/savings-calculator/Critical shared infrastructure had zero test coverage, including:
Although Jest + React Testing Library were properly configured, they were severely underutilized.
This created risks in:
✅ Solution Implemented
Implemented comprehensive unit and integration-level test coverage across all common infrastructure.
1️⃣ Utilities Testing
coverImageUtil)Covered:
commonUtils.jsformatCount.tscoverImageUtil.js2️⃣ Custom Hooks Testing
Used:
renderHookwaitForjest.mock()global.fetchmockingValidated:
Covered:
useFetch.jsuseCacheResponse.tsuseContributors.jsuseFeaturedPlays.jsuseGitHub.jsuseLikePlays.jsuseLocalStorage.js3️⃣ Search System Testing
Fully validated:
Covered:
search-helper.jsParseQueryQueryDBTranslator4️⃣ Service Layer Integration Tests
Covered:
plays.jsAll tests are passing successfully.
📁 Files Created
Total: 13 new test files
Total: 67 test cases
🛠 File Changes Summary
➕ Added
.test.js/.test.tsfiles under:src/common/hooks/__tests__/src/common/utils/__tests__/src/common/services/__tests__/src/common/search/__tests__/⚙ Updated (CI)
src/common/minimum coverage: 60%🔄 Workflow Diagram
Test Coverage Implementation Workflow
flowchart TD A[Identify Untested Infrastructure] --> B[Categorize by Type] B --> C[Utilities] B --> D[Hooks] B --> E[Search] B --> F[Services] C --> G[Write Pure Function Tests] D --> H[RenderHook + Async Tests] E --> I[Query Parsing Assertions] F --> J[Mock GraphQL Layer] G --> K[Run Jest] H --> K I --> K J --> K K --> L[Verify Coverage Threshold] L --> M[CI Enforcement]@project admin and @mentor please add apertre and difficulty lebels before merge