Skip to content

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

Merged
kobenguyent merged 2 commits intocodeceptjs:3.xfrom
myrepojuly:3.x
Aug 26, 2025
Merged

Fix: JSONResponse helper to preserve original onResponse behavior (Fixes #5042)#5106
kobenguyent merged 2 commits intocodeceptjs:3.xfrom
myrepojuly:3.x

Conversation

@myrepojuly
Copy link
Contributor

@myrepojuly myrepojuly commented Aug 24, 2025

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
Copilot AI reviewed Aug 24, 2025
View reviewed changes
Copy link
Contributor

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

kobenguyent commented Aug 24, 2025

Please add tests if that's possible. Thanks.

@myrepojuly
Copy link
Contributor Author

myrepojuly commented Aug 24, 2025

@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

kobenguyent commented Aug 24, 2025

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

@myrepojuly
Copy link
Contributor Author

myrepojuly commented Aug 24, 2025

@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
@myrepojuly
Fix: JSONResponse helper to preserve original onResponse behavior (Fixes
e371c09 
 #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 <myrepo1987@gmail.com>
@kobenguyent kobenguyent mentioned this pull request Sep 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

REST helper config: onResponse provided in config does not work

2 participants

@myrepojuly @kobenguyent