Skip to content

Conversation

@jiji-hoon96
Copy link
Contributor

Description

This change aims to improve the test coverage of the project. To enhance the stability and maintainability of the codebase, test codes have been added and improved for areas that previously lacked sufficient testing.

Changes

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Before

스크린샷 2025-05-23 오후 5 43 58

After

스크린샷 2025-05-23 오후 5 43 39

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have performed a self-review of my own code.
  • My code is commented, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

Further Comments

I've been working on improving the test coverage for our recent project. While we've made significant progress overall, I'm encountering some difficulties in writing tests for specific parts of a few files and would appreciate your guidance.

I'm particularly struggling to increase coverage for the following files:

src/context/reducer.ts

Current Status

  • Function coverage is reported as 0%.
  • Lines 99 and 147 are not covered by tests.

Detailed Issues

  • Line 99: This line is within the SET_OVERLAY_STATE action type's logic, specifically a conditional statement that removes an existing overlay's state if an overlay with the same overlayId already exists and the keepPrevious option is false. I'm finding it challenging to construct the precise action payload and previous state to satisfy this condition. Specifically, it's difficult to create a scenario where the draft.overlays.splice(existingOverlayIndex, 1) code is executed.
  • Line 147: This line is part of the REMOVE_OVERLAY_STATE_BY_ID action type's logic, responsible for finding and removing an overlay corresponding to a specific overlayId. It's unclear whether this line is only executed under specific conditions (e.g., multiple overlays with the same overlayId) or if a test case dispatching this action is simply missing. I'd like to clarify the conditions under which draft.overlays.splice(overlayIndex, 1) is executed.

Attempts to Resolve

  • I've tried directly calling the reducer function with various SET_OVERLAY_STATE and REMOVE_OVERLAY_STATE_BY_ID action objects. However, I haven't been able to create the exact combination of state and action that meets the specific conditions for lines 99 and 147.
  • For example, to cover line 99, I attempted to create a situation where keepPrevious: false and an overlayId was duplicated, but I couldn't reproduce the exact logic flow in the test code that would trigger the splice call.
  • There are no external dependencies, so jest.mock was not used.

src/utils/create-use-external-events.ts

Current Status

  • Branch coverage is at 92.3%, with line 6 remaining uncovered.

Detailed Issues

  • Line 6: This is an if (!targetElement) conditional statement within the useEffect hook. This branch is taken when targetElement is null or undefined, which could typically occur if the component using the custom hook is unmounted or if targetElement cannot be found due to specific conditions. It's proven difficult to intentionally create a scenario in the test environment where targetElement does not exist and to control the flow to hit this branch.

Attempts to Resolve

  • I've used the create-use-external-events hook within a test component and tried to trigger relevant DOM events using fireEvent. However, this approach typically covers scenarios where targetElement exists, thus not satisfying the if (!targetElement) condition on line 6.
  • I attempted to use jest.spyOn to manipulate the return value of document.getElementById (or similar DOM selector functions) to null, thereby simulating a missing targetElement. However, due to the hook's internal logic and timing issues, I haven't been able to reliably cover this branch.

I would be grateful for any advice or alternative approaches you might have for effectively increasing test coverage for these problematic areas. Guidance on testing strategies for complex conditional logic or code that depends on specific DOM states would be particularly helpful.

@jiji-hoon96 jiji-hoon96 requested a review from jungpaeng as a code owner May 23, 2025 08:50
@changeset-bot
Copy link

changeset-bot bot commented May 23, 2025

🦋 Changeset detected

Latest commit: 3ca8dcd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
overlay-kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented May 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
overlay-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 23, 2025 8:52am

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.85%. Comparing base (3f892fd) to head (3ca8dcd).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
+ Coverage   93.68%   99.85%   +6.16%     
==========================================
  Files          12       15       +3     
  Lines         364     1337     +973     
  Branches       88      260     +172     
==========================================
+ Hits          341     1335     +994     
+ Misses         22        2      -20     
+ Partials        1        0       -1     
Components Coverage Δ
overlay-kit 99.85% <100.00%> (+6.16%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vercel
Copy link

vercel bot commented Oct 12, 2025

@jungpaeng is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@autofix-troubleshooter
Copy link

Hi! I'm the autofix logoautofix.ci troubleshooter bot.

It looks like you correctly set up a CI job that uses the autofix.ci GitHub Action, but the autofix.ci GitHub App has not been installed for this repository. This means that autofix.ci unfortunately does not have the permissions to fix this pull request. If you are the repository owner, please install the app and then restart the CI workflow! 😃

@jungpaeng jungpaeng force-pushed the test/coverage-improvement branch from a61eb94 to 8f582ab Compare October 12, 2025 16:23
Copy link
Member

@jungpaeng jungpaeng left a comment

Choose a reason for hiding this comment

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

Thanks!

@jungpaeng jungpaeng merged commit 6d35a52 into toss:main Oct 12, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants