Skip to content

test(config): cover error propagation and live config loading - #1096

Merged
decyjphr merged 3 commits into
yadhav/fix-recent-issuesfrom
decyjphr-config-error-audit
Sep 29, 2026
Merged

decyjphr merged 3 commits into
yadhav/fix-recent-issuesfrom
decyjphr-config-error-audit

Conversation

@decyjphr

Copy link
Copy Markdown
Collaborator

Why

PR #1052 addresses config-read failures being masked by a TypeError, but yadhav/fix-recent-issues already preserves the original error: commit 5fac715f271114a0d23d020cea0a0eed045fde0d removed the swallowing catch, and fbd78627ff6f380f472245497ceb0cf7b36e5e93 retained that behavior during the Octokit migration. The missing piece is dedicated regression coverage, not another production fix.

Approach

  • Adapt the tests from source commit fe88b3a4de6fb0ae05fd978d0fa07adba0c8431b, covering original error identity for HTTP 403/500 and network failures, 404-to-null behavior, directories/symlinks, YAML parsing and empty-config semantics, and configured owner/repo/path/ref handling.
  • Exercise the actual full-sync entry point and NOP reporting so original errors reach failed checks and PR comments without becoming a TypeError.
  • Add smoke phase 23 for real config loading, empty YAML, directory responses, and missing-file/ref 404s. It owns a temporary branch, refuses to replace an existing branch, and attempts cleanup even on failure. Document running it alongside existing validation phase 10.

No production code, dependencies, or batching/result-reporting behavior changes. The branch is based on target commit 188c604929201774699fd1d8dabd445358cee9e8.

Validation

Using Node 22.12.0 / npm 10.9.0:

  • Original seven PR tests passed against unchanged target production code.
  • npm run test:unit -- --runInBand --reporters=default --silent: 821 passed, 12 skipped; focused loader/reporting tests: 26 passed.
  • New tests pass Standard and ESLint; smoke syntax and diff checks pass.
  • node smoke-test.js --phase 10,23: exact 0 -> 10 -> 23 -> 9, 10 passed, 0 failed, with no fatal error or timeout. Real server responsiveness and check conclusions were verified. Independent before/after snapshots matched, preserving 15 existing refs, 12 property definitions, and the admin ruleset; owned resources/processes were cleaned up.

Known limitations

Seven integration suites fail before executing tests because of the existing Probot ESM/Jest incompatibility. The smoke script retains nine baseline lint errors, with no new diagnostics.

Live validation used an uncommitted session-only auth/API/webhook guard. A retrospective audit found Probot reloaded .env after the preload, so intended blank enterprise/cron environment overrides were not proven effective: enterprise discovery ran, and cron registration was not instrumented. Saved guarded GitHub traffic contains only test-installation token requests and permitted routes, but is not an all-network trace. Forwarding was intentionally enabled and the server used IPv6 loopback. These qualifications do not change the recorded functional assertions or cleanup results. Injected 403/500/network scenarios were local tests, not counted as live smoke assertions.

decyjphr and others added 2 commits September 28, 2026 23:26
Production behavior from PR #1052 is already present via 5fac715. Adapt its missing regression tests, verify original errors through full-sync and NOP reporting, and add isolated live config-loading smoke phase 23.

Source tests adapted from fe88b3a.

Co-authored-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve smoke phases 21 through 24 and all startup-safety documentation. Adapt config error assertions to the target branch's full-sync error aggregate while retaining original error identity.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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 review overview

🟡 Changes recommended

Update smoke-test cleanup and remove the unused test fixture.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds regression and smoke coverage for config-loading error propagation without changing production code.

Changes:

  • Expands config, YAML, path, and error-identity tests.
  • Adds full-sync and NOP reporting tests.
  • Adds smoke-test Phase 23 and documents its usage.
File Summary Review notes
test/​unit/​lib/​configManager.test.js Config loader regression coverage —
test/​unit/​index-config-errors.test.js Full-sync and NOP error reporting tests Remove unused context fixture.
smoke-test.js Live config-loading smoke phase Use deleteBranch for idempotent cleanup.
README.md Documents Phase 23 —

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread smoke-test.js Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@decyjphr
decyjphr merged commit 55b52df into yadhav/fix-recent-issues Sep 29, 2026
2 checks passed
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.

2 participants