[jsweep] Clean action_setup_otlp.cjs#34578
Conversation
- Remove unused `path` module dependency; use `require('./send_otlp_span.cjs')` directly
- Add explicit `fileLabel` parameter to `writeEnvLine` to remove fragile
`filePath === process.env.GITHUB_OUTPUT` comparison
- Add 6 new tests covering INPUT_PARENT_SPAN_ID handling (pass-through,
lowercase normalisation, logging, hyphen-form fallback, env-var normalisation)
All 45 tests pass (39 original + 6 new).
Co-authored-by: Copilot <[email protected]>
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #34578 does not have the 'implementation' label and has 0 new lines in default business logic directories (threshold: 100). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR performs a small cleanup of the action_setup_otlp.cjs helper used by setup.sh and actions/setup/index.js, and extends the unit tests to cover previously untested INPUT_PARENT_SPAN_ID behavior.
Changes:
- Simplify
send_otlp_span.cjsloading by switching to a relativerequire()and dropping the unusedpathdependency. - Make
writeEnvLinelogging more explicit by passing afileLabelrather than inferring it from runtime path comparisons. - Add test coverage for
INPUT_PARENT_SPAN_IDparsing/normalization, including hyphen/underscore env-var forms.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/action_setup_otlp.cjs | Removes path usage, simplifies require, and makes env/output write logging explicit via fileLabel. |
| actions/setup/js/action_setup_otlp.test.cjs | Adds tests covering INPUT_PARENT_SPAN_ID normalization, logging, and hyphen-form fallback behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (6 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — approving with one minor suggestion.
📋 Key Themes & Highlights
Key Themes
fileLabelrefactor: Clean improvement — the explicit parameter replaces a fragile side-channel env comparison. Only gap: no test verifies that the correct label ("GITHUB_OUTPUT"/"GITHUB_ENV") appears in the logged output at each call site.- New
INPUT_PARENT_SPAN_IDtests: Six well-structured tests covering the happy path, normalisation, and the hyphen-form fallback. Good addition.
Positive Highlights
- ✅ Removing the
pathmodule dependency is a clean simplification - ✅
fileLabelparameter makeswriteEnvLineself-documenting and removes implicit coupling toprocess.env - ✅ New tests follow the existing suite conventions and are easy to read
- ✅ All CI checks pass per PR description
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
|
|
||
| it("should set INPUT_PARENT_SPAN_ID env var from the hyphen form when only hyphen form is present", async () => { | ||
| delete process.env.INPUT_PARENT_SPAN_ID; | ||
| process.env["INPUT_PARENT-SPAN-ID"] = "aabbccddeeff0011"; |
There was a problem hiding this comment.
[/tdd] No test verifies the log output from the updated writeEnvLine after the fileLabel refactor.
The PR's main production change is replacing the filePath === process.env.GITHUB_OUTPUT ternary with an explicit fileLabel string — but no test asserts that console.log is called with "GITHUB_OUTPUT" or "GITHUB_ENV". If the wrong label is passed at a call site, tests won't catch it.
💡 Suggested addition
it("should log GITHUB_OUTPUT when writing trace-id to github output", async () => {
process.env.OTEL_TRACE_ID = "a".repeat(32);
await run();
expect(console.log).toHaveBeenCalledWith(
expect.stringContaining("written to GITHUB_OUTPUT")
);
});
it("should log GITHUB_ENV when writing trace context to github env", async () => {
process.env.OTEL_TRACE_ID = "a".repeat(32);
process.env.GITHUB_ENV = "/tmp/github_env";
await run();
expect(console.log).toHaveBeenCalledWith(
expect.stringContaining("written to GITHUB_ENV")
);
});One test per file destination is enough to lock in the intent of the refactor.
Summary
This PR cleans up
action_setup_otlp.cjsas part of the jsweep initiative. It removes an unusedpathmodule import, replaces a dynamicpath.join(__dirname, ...)require with a static relative require, and refactorswriteEnvLineto accept an explicitfileLabelparameter instead of inferring the destination file name by comparing paths againstprocess.env.GITHUB_OUTPUT.Changes
actions/setup/js/action_setup_otlp.cjspathimport, static relative require, explicitfileLabelparam onwriteEnvLineactions/setup/js/action_setup_otlp.test.cjsINPUT_PARENT_SPAN_ID handlingtest suite covering absent, present, normalised, hyphen-fallback, and env-var-backfill casesMotivation & context
Part of the ongoing
jsweepcleanup pass across theactions/setup/js/files. Thepathimport was dead code, the dynamicrequirewas unnecessary complexity, and the implicit destination-file inference inwriteEnvLinewas fragile. MakingfileLabelexplicit improves readability and removes a hidden dependency onprocess.env.GITHUB_OUTPUTcomparisons inside the helper.Impact assessment
INPUT_PARENT_SPAN_ID handlingtest suite added covering five scenarios: absent input, present value, uppercase normalisation, hyphen-form fallback, and env-var backfillTesting
The updated test file adds a dedicated
INPUT_PARENT_SPAN_ID handlingsuite that exercises: (1) missing input being handled gracefully, (2) a valid parent span ID being written correctly, (3) uppercase input being normalised, (4) the hyphen-form alias falling back as expected, and (5) an env-var backfill path being exercised when the value is already set in the environment.