Skip to content

Conversation

myrepojuly
Copy link
Contributor

Motivation/Description of the PR

  • Description of this PR, which problem it solves: Problem:
    The current implementation overrides any custom onResponse handler that may have already been set in the user's configuration, causing their logic to be lost.

Suggested Fix:
Preserve the original onResponse handler by wrapping it, rather than replacing it:

Applicable helpers:

  • Playwright
  • Puppeteer
  • WebDriver
  • REST
  • FileHelper
  • Appium
  • TestCafe

Applicable plugins:

  • allure
  • autoDelay
  • autoLogin
  • customLocator
  • pauseOnFail
  • coverage
  • retryFailedStep
  • screenshotOnFail
  • selenoid
  • stepByStepReport
  • stepTimeout
  • wdio
  • subtitles

Type of change

  • 🔥 Breaking changes
  • 🚀 New functionality
  • 🐛 Bug fix
  • 🧹 Chore
  • 📋 Documentation changes/updates
  • ♨️ Hot fix
  • 🔨 Markdown files fix - not related to source code
  • 💅 Polish code

Checklist:

  • Tests have been added
  • Documentation has been added (Run npm run docs)
  • Lint checking (Run npm run lint)
  • Local tests are passed (Run npm test)

@kobenguyent kobenguyent requested a review from Copilot August 24, 2025 04:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the JSONResponse helper where setting the onResponse handler was overriding any existing custom onResponse handler in the user's configuration. The fix preserves the original onResponse behavior by wrapping it instead of replacing it.

Key changes:

  • Preserve existing onResponse handlers by storing and calling the original function
  • Maintain backward compatibility while fixing the override issue

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@kobenguyent
Copy link
Collaborator

Please add tests if that's possible. Thanks.

@myrepojuly
Copy link
Contributor Author

@kobenguyent - I’ve added tests under ./test/unit/helper/, but some existing tests are failing, and I can’t push.
Is it okay to use --no-verify to bypass the husky's pre-push check?

430 passing (16s)
1 pending
3 failing

1) FileSystem
   should check file contents:
   expected contents of fs_sample.txt to equal:
   "A simple file
   for FileSystem helper
   test"

2) HTML module
   #splitByChunks
   should cut long HTMLs into chunks and add paths into them:
   AssertionError: expected array length of 21, but got 22

3) utils
   #screenshotOutputFolder
   returns the joined filename for filename only:
   AssertionError: expected path to be relative, but got absolute Windows path

@kobenguyent
Copy link
Collaborator

@myrepojuly i guess that's fine as we would run tests on GitHub anyway.

@myrepojuly
Copy link
Contributor Author

@kobenguyent - thank you.. Added the unit tests...

@kobenguyent kobenguyent merged commit e94fc98 into codeceptjs:3.x Aug 26, 2025
14 checks passed
Copilot AI pushed a commit that referenced this pull request Sep 1, 2025
 #5042) (#5106)

* Fix: JSONResponse helper to preserve original onResponse behavior (Fixes #5042)

* Add unit tests for REST onResponse hook functionality:Fixes #5042

---------

Co-authored-by: myrepo <[email protected]>
@kobenguyent kobenguyent mentioned this pull request Sep 22, 2025
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.

REST helper config: onResponse provided in config does not work
2 participants