Skip to content

test: preserve generation diagnostics and wait for tray probe readiness - #7038

Merged
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:test/7017-generation-tray-hardening
Oct 13, 2026
Merged

lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:test/7017-generation-tray-hardening

Conversation

@luvs01

@luvs01 luvs01 commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Follow up on the two test improvements set aside from #7017, as suggested by the maintainer.

  • Preserve bounded stdout/stderr and startup-phase diagnostics from the generation-race children, detect exits before readiness, and reap owned children when collection fails. Three real-child regressions cover early exit, large-output draining, and cleanup. The 10-second deadlines and exactly-one-callback assertions stay unchanged; this improves timeout diagnosis rather than claiming to fix slow startup.
  • Make the tray fixture follow production initialization before measuring maintenance, then wait for the child's own PID signal instead of assuming it is ready after 500 ms. The Online child deliberately waits 1,500 ms before reporting readiness. The 250 ms maintenance limit and termination, reaping, and single-launch assertions stay unchanged.

The diff is limited to three test/fixture files (+155/-40). It carries the unchanged patches from 3e763570ac0136e527d51aaf3d59b39cdb4ad69d and 23b4333ca39eb18b42d8dfe200820f0f583169ac onto dev a5cd5a2b832d727cfef5e77c48547b47b1cbeebb, preserving their authorship. The config-bound product changes already merged in #7029 and other reader changes are not part of this PR.

Verification

Local checks were run on Linux at 585019db72adc08725c2b838788236aea7eef157, using Bun 1.4.2 for runtime commands and the repository's pinned Bun 1.4.3 test runner.

bun run test --parallel=1 \
  ./tests/codex-integration/codex-config-generation.test.ts \
  ./tests/windows/windows-tray.test.ts \
  ./tests/test-layout.test.ts \
  ./tests/test-layout-tooling.test.ts \
  ./tests/ci-workflows/file-size-ratchet.test.ts \
  ./tests/ci-workflows/structure-ssot.test.ts
bun run typecheck
bun run structure:check
bun run privacy:scan
git diff --check a5cd5a2b832d727cfef5e77c48547b47b1cbeebb..HEAD

The focused run reported 122 pass, 0 fail across six files, including all 15 generation cases. Five Windows-only cases return early on Linux, so their reported passes do not establish native Windows coverage. Typecheck, structure, privacy, and diff checks passed. The repository typecheck covers src/.

I also checked the two TypeScript test files explicitly:

bun x tsc --ignoreConfig --noEmit --target ESNext --module ESNext \
  --moduleResolution bundler --resolveJsonModule --types bun-types \
  --strict --esModuleInterop --skipLibCheck \
  tests/codex-integration/codex-config-generation.test.ts \
  tests/windows/windows-tray.test.ts

This expanded check is not clean: the unchanged dev baseline reports four errors and this head reports two, with no new diagnostics. The remaining errors are SyncSubprocess.signal in scripts/test-temp-lock.ts:78 and a fixture missing bunRuntimeSource in tests/windows/windows-tray.test.ts:167. The two baseline generation pipe-type errors disappear with the carried patch. Unrelated type fixes are left out of this change.

Native Windows validation now passed for this exact head in Cross-platform CI run 38378944192, using Bun 1.4.3. The workflow checked out PR merge 753e9660bbe07e8443888dd669b6f5e32052a45e, whose tree is identical to head 585019db72adc08725c2b838788236aea7eef157.

  • Windows 6/9: all 15 generation cases passed, including exactly-one callback, early-exit stderr, bounded-output draining, and child cleanup.
  • Windows 8/9: 28 tray cases passed, 0 failed. The native lifecycle case completed all six scenarios: Offline, Online (with the 1,500 ms readiness delay), BadgeOffline, BadgeOverflowStdout, BadgeOverflowStderr, and BadgeStaleFailure. The 250 ms maintenance boundary remained enforced.

These are primary test-batch results on the new head, with no attribution rerun; the historical CI pass for 23b4333c is not used as validation.

The full 2,268-module suite was not run locally. For this three-file test follow-up, I used focused real-child tests and the layout, size, and structure checks. CI passed all four Linux test shards and nine native Windows test shards, and the final aggregate ci job passed. The completed run has 22 successful jobs and 10 path-filtered skips, with no failures. Path-filtered jobs, including macOS, were skipped and are not claimed as validated. This PR remains draft with the review-readiness checklist below unchecked.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. No user-facing behavior changes here.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. The diff only changes test diagnostics and fixtures; no authentication, permission, workflow, or production security behavior changes.

Co-authored-by: Epinephrine 27862058+luvs01@users.noreply.github.com
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

luvs01 and others added 2 commits October 13, 2026 09:18
Drain bounded stdout and stderr from generation-race children, identify exits before readiness, and report per-child missing files and import phases. Reap owned children in a finally block when readiness or outcome collection fails.

Keep the 10-second readiness and child deadlines and the exact-one-callback generation assertions unchanged. Add real-child regressions for early exit, bounded capture, and cleanup. This records evidence for a startup timeout; it does not claim that timeout is fixed.

Reconstruct the test-only follow-up from lidge-jun#7017 on current dev.
Source-commit: 3e76357
Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com>
Perform production initialization and its initial launch tick before measuring maintenance, then wait for the actual child PID readiness signal instead of a fixed sleep. Retain the 250 ms maintenance bound, process reaping assertions, and delayed Online readiness coverage.

Reconstruct the test-only follow-up from lidge-jun#7017 on current dev. Native Windows validation of this new candidate is still required; prior source-commit CI is not evidence for this candidate.
Source-commit: 23b4333
Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Oct 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration (dev-only bypass per MAINTAINERS.md), not an independent approval.

Exact head 9ca09d95978699526008043ad1961fddb4b03d91 (current dev merged in, no force-push). Hosted Cross-platform CI on this head: all required jobs passed (Linux 4/4, Windows 9/9, gates, aggregate ci); path-filtered jobs skipped. Windows 6/9 and 8/9 carry the generation and tray cases this PR changes.

Independent exact-head review (gpt-6.1-sol, read-only): MERGE-OK, no findings. Combined tree with #7036/#7038/#7039 on current dev: typecheck, structure:check, privacy:scan, file-size ratchet, test-layout and core/Lab boundary passed; 423 focused tests across 19 files passed locally. No outstanding review threads or change requests.

@lidge-jun
lidge-jun marked this pull request as ready for review October 13, 2026 14:42
@lidge-jun
lidge-jun merged commit 76b0d0e into lidge-jun:dev Oct 13, 2026
38 checks passed
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T14:49:23.905895Z 9ca09d9 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ca09d9597

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +101 to +103
if (Date.now() >= deadline) {
const missing = paths.filter(path => !existsSync(path));
throw new Error(`timed out waiting for ${missing.join(", ")}; children: ${guardRaceDiagnostics(children)}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Set the test timeout beyond the child deadline

For local focused runs that omit an explicit timeout (for example, bun run test --parallel=1 ./tests/codex-integration/codex-config-generation.test.ts), scripts/test.ts's resolveBunTestArgs adds isolation and parallelism but no per-test timeout. I checked bun test --help, which documents --timeout=<val> ... default is 5000; therefore, when a generation child really fails to become ready, Bun aborts the test at 5 seconds before this 10-second branch can emit the newly captured stdout/stderr and phase diagnostics. Give this process-spawning test an explicit timeout long enough for the child deadline plus reaping, or move the diagnostic deadline below the runner timeout.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants