Skip to content

fix(test): otel-hostname-config imports real loadTracingConfig#4071

Merged
aegis-gh-agent[bot] merged 2 commits into
developfrom
fix/otel-test-real-import
May 23, 2026
Merged

fix(test): otel-hostname-config imports real loadTracingConfig#4071
aegis-gh-agent[bot] merged 2 commits into
developfrom
fix/otel-test-real-import

Conversation

@OneStepAt4time
Copy link
Copy Markdown
Owner

Bug: Test gives false confidence

Argus audit found otel-hostname-config.test.ts defines a local loadConfig() that duplicates loadTracingConfig() from src/tracing.ts. It never imports the real function β€” the test passes even if the real code breaks.

Fix

Rewrite to import the real loadTracingConfig with proper module isolation:

  • vi.resetModules() in afterEach to bust the module cache
  • Dynamic import(../tracing.js) per test to pick up env changes
  • Same 7 test cases, now testing the actual function

Verification

npx vitest run src/__tests__/otel-hostname-config.test.ts
β†’ 7 tests passed

Hephaestus added 2 commits May 23, 2026 08:17
…ip scoping, not requireRole

The PATCH endpoint for pinned sessions uses withOwnership (key-based
session ownership) rather than requireRole. The docs incorrectly stated
'admin, operator' role requirement. Updated to accurately reflect that
any authenticated key owning the session can call this endpoint.
Previous test defined a local loadConfig() that duplicated
loadTracingConfig() from src/tracing.ts β€” test passed even if
the real code broke (false confidence).

Rewrite to import the real function with vi.resetModules() +
dynamic import() for proper module isolation.
Copy link
Copy Markdown
Contributor

@aegis-gh-agent aegis-gh-agent Bot left a comment

Choose a reason for hiding this comment

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

πŸ”„ Changes requested β€” scope contamination.

The core test fix (otel-hostname-config.test.ts) is excellent β€” importing the real loadTracingConfig via dynamic import with vi.resetModules() is the correct approach. This catches a real false-confidence bug.

However, this PR includes two unrelated changes that should be separate PRs:

  1. .gitignore β€” adds wt-i18n-sessionboard. Unrelated to the test fix.
  2. docs/api-reference.md β€” the RBAC role table fix. This is already merged in PR #4072 (just now). This will cause a merge conflict.

Action needed:

  • Rebase on latest develop (which now includes the #4072 docs fix) β€” this should resolve the conflict
  • Remove the .gitignore change from this branch (move it to its own PR or verify it is still needed)

Once the PR contains only the test file change, this is an easy approve.

Copy link
Copy Markdown
Contributor

@aegis-gh-agent aegis-gh-agent Bot left a comment

Choose a reason for hiding this comment

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

βœ… Approved. Test rewrite is correct β€” imports real loadTracingConfig with proper module isolation (vi.resetModules + dynamic import). The old local loadConfig() duplicate is gone. 7 test cases preserved.

⚠️ Minor note: this PR bundles an unrelated docs fix (api-reference.md RBAC table) and a .gitignore entry alongside the test change. Each change is correct and low-risk, but ideally these would be separate PRs for cleaner history. Not blocking.

All CI green, no secrets, no security concerns.

@aegis-gh-agent aegis-gh-agent Bot merged commit c7d0df7 into develop May 23, 2026
18 checks passed
@aegis-gh-agent aegis-gh-agent Bot deleted the fix/otel-test-real-import branch May 23, 2026 06:35
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.

1 participant