Skip to content

fix(hooks): emit hook.failed when a helper suite hook throws - #5684

Open
luantaraschi wants to merge 2 commits into
codeceptjs:4.xfrom
luantaraschi:fix/helper-suite-hook-failed
Open

fix(hooks): emit hook.failed when a helper suite hook throws#5684
luantaraschi wants to merge 2 commits into
codeceptjs:4.xfrom
luantaraschi:fix/helper-suite-hook-failed

Conversation

@luantaraschi

Copy link
Copy Markdown

Motivation/Description of the PR

Resolves #5660.

@mirao's trace is correct and I followed it through the code. event.hook.failed is only emitted by fireHook(), which is only reached from the injected() wrapper used for test-file-defined BeforeSuite() / AfterSuite(). A custom helper's _beforeSuite() / _afterSuite() takes a different route:

lib/mocha/ui.js:106 registers suite.beforeAll('codeceptjs.beforeSuite', suiteSetup(suite))suiteSetup emits event.suite.beforelib/listener/helpers.js:30 queues runAsyncHelpersHook('_beforeSuite', ...) on the recorder. When that throws, it surfaces in suiteSetup's recorder.errHandler, which called doneFn(err) and nothing else. No hook.failed, so junitReporter's listener never fired and the report stayed empty.

Both error handlers now emit the matching hook object before calling done. I passed the mocha suite straight through, which is what fireHook() already does, so hook.ctx.test and hook.hookName come out the same shape junitReporter already consumes (['BeforeSuite', 'AfterSuite'].includes(hook.hookName)).

I did not reuse fireHook() here: it derives the hook kind from suite.ctx?.test?.title?.match(/"([^"]*)"/)[1], and these hooks are titled codeceptjs.beforeSuite, which has no quoted segment, so that match returns null and the indexing throws.

Type of change

  • 🐛 Bug fix

Checklist:

  • Tests have been added
  • Documentation has been added (Run npm run docs) — N/A, no public API change
  • Lint checking (Run npm run lint)
  • Local tests are passed (Run npm test)

Two tests appended to test/unit/mocha/asyncWrapper_test.js, one per hook. They queue a throwing helper method the same way runAsyncHelpersHook does and assert both that done still receives the error and that hook.failed carries the right hookName. Both fail on the commit before this change with hook.failed was emitted.

Full unit suite on Windows: 758 passing / 13 failing before, 760 passing / 11 failing after. The 11 remaining are pre-existing path assertions that expect POSIX paths and see a C: drive letter (utils_test.js, utils/trace_test.js), identical with and without this change.

Related: #5683 fixes the other half of the junit reporting gap @mirao reported, the suite timestamp.

A custom helper's `_beforeSuite()` / `_afterSuite()` is queued on the
recorder from the `event.suite.before` / `event.suite.after` listeners in
lib/listener/helpers.js. That path runs inside suiteSetup/suiteTeardown,
not inside the `injected()` wrapper, and only `injected()` calls
`fireHook()`. So a failing helper lifecycle method rejected the mocha hook
without ever emitting `event.hook.failed`, and reporters that listen for
it, junitReporter among them, recorded nothing.

Both error handlers now emit the matching hook object before calling
done, so `hookName` reads BeforeSuite or AfterSuite exactly as it does for
the test-file-defined hooks.

Closes codeceptjs#5660
Copilot AI lite review requested due to automatic review settings August 7, 2026 12:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@luantaraschi

Copy link
Copy Markdown
Author

The red appium job on this PR was not about the change, and it should be gone now. Recording what it was in case it comes up on other fork PRs.

The last CI here ran on 2026-08-07 and the Android job failed at the Sauce Labs handshake:

{"detail":"Authorization failed"}
WebDriverError: failed serving request POST /wd/hub/session: Unauthorized
Appium "before all" hook in "Appium"  Can't connect to WebDriver.

That is a missing credential, not a broken test. Fork pull requests do not receive SAUCE_USERNAME / SAUCE_ACCESS_KEY, and neither do Dependabot ones, so the job cannot pass from either. Grouping the last hundred Android runs by trigger shows the split cleanly: pushes to 4.x and pull requests whose head branch lives in the repo pass, while every Dependabot pull request in the same window fails at the same line.

You already fixed this on 2026-08-12 in 6a5ce8d3 / eb1bcdc5, "fix(ci): dont run appium tests for PRs from forked repos", which added the github.event.pull_request.head.repo.full_name == github.repository guard. This PR's run predated it by five days, which is the whole reason it looked red.

So I pressed Update branch to pick up the guard rather than leave a red X sitting on an approved PR. The Android job is skipped on the new head, as intended. The fresh runs are sitting at action_required, which is the fork-contributor gate, so they need an approval from a maintainer before they will report.

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.x: junitReporter still doesn't record helper _beforeSuite()/_afterSuite() failures (follow-up to #5645)

3 participants