Skip to content

fix(junitReporter): stamp suites with their real start time - #5683

Open
luantaraschi wants to merge 2 commits into
codeceptjs:4.xfrom
luantaraschi:fix/suite-started-at
Open

fix(junitReporter): stamp suites with their real start time#5683
luantaraschi wants to merge 2 commits into
codeceptjs:4.xfrom
luantaraschi:fix/suite-started-at

Conversation

@luantaraschi

@luantaraschi luantaraschi commented Aug 7, 2026

Copy link
Copy Markdown

Motivation/Description of the PR

Resolves #5668.

junitReporter writes each <testsuite timestamp> from suite.startedAt (lib/plugin/junitReporter.js:135), but nothing ever set that field. Confirming @mirao's read: startedAt is assigned in exactly one place, lib/listener/steps.js:20, and only to individual tests. Mocha's Suite does not carry it. So toIso() always fell through to its new Date() fallback and every suite got stamped with the moment the XML was serialized, which is after that suite and its AfterSuite had already finished.

The steps listener now stamps the suite on event.suite.before, mirroring what it already does for tests two lines below.

One thing worth flagging: the existing junitReporter_test.js builds its fixtures with startedAt: Date.now() on each suite object, so the tests were exercising a field that production never produced. That is why the reporter's own suite passed while the output was wrong. I left those fixtures alone since they now describe reality, and put the new coverage on the listener instead.

Scope note: this covers the in-process run. Under run-workers, lib/mocha/test.js serializes a test's parent down to { title } and drops startedAt from the test payload as well, so a suite timestamp cannot cross that boundary without changing the worker payload. That looked like a separate change, so I left it out rather than widen this PR.

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 in test/unit/listener/steps_suite_started_at_test.js. They fail on the commit before this change with expected undefined to be a number, and pass with it.

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

junitReporter writes each `<testsuite timestamp>` from `suite.startedAt`,
but nothing ever set that field. Mocha's Suite does not carry it, and
`startedAt` was only assigned to individual tests, in
lib/listener/steps.js. So `toIso()` always fell through to its
`new Date()` fallback and every suite was stamped with the moment the XML
was serialized, which is after the suite (and its AfterSuite) finished.

The steps listener now stamps the suite on `event.suite.before`, mirroring
what it already does for tests.

Note this covers the in-process run. Under `run-workers` the parent only
receives `{ title }` for a test's parent suite, so the timestamp cannot
survive that boundary without changing the worker payload.

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

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.

@luantaraschi

Copy link
Copy Markdown
Author

Unit tests (26.x) went red after I pressed Update branch here this morning, and I do not think it belongs to this change. Here is what I checked, including the one way this PR could plausibly have caused it.

The failure is a single assertion:

✖ els #element should fail on timeout if timeout is set
  expected 'should have thrown error' to include 'was interrupted on timeout 10ms'

That lives in test/unit/els_test.js and the message comes from lib/recorder.js. A 10 ms budget on a loaded runner is a thin margin.

The same code passed this job a week ago. Run 31176426017 on this branch, sha 5205311b, was green on Unit tests (26.x) on August 7. The only thing that changed since is the 4.x merge from Update branch.

The PR that took the same merge in the same minute is green. #5684 got the identical 4.x merge and its Unit tests (26.x) passed.

The one real suspicion, checked. My new test calls event.cleanDispatcher() and recorder.reset(), which are global, so a file-order interaction with els_test.js was worth ruling out rather than waving away. Mocha loads test/unit/els_test.js at position 18 of 64 and test/unit/listener/steps_suite_started_at_test.js at position 28, so my file runs after that suite has already finished. It cannot reach it.

Locally, on Windows with Node 22.20.0:

tree passing failing
this PR, 45a56b4c 760 11
4.x control 758 11

The same eleven both times, all of them Windows-specific (workers, sharding, screencast, trace, MCP), and els #element is not among them either way. The +2 is exactly the two tests this PR adds.

So a re-run of that job should clear it. Sorry for the noise, the Update branch was to pick up eb1bcdc5 so the fork-side Appium job would be skipped, which it now is.

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.

junitReporter: per-<testsuite> timestamp uses report-write time, not suite-start time

2 participants