[jsweep] Clean action_conclusion_otlp.cjs#24669
Merged
Conversation
- Remove unused path require; use relative require('./send_otlp_span.cjs')
- Hold module reference (avoid destructuring) to enable CJS mock patching in tests
- Add 11 comprehensive tests covering: skip behavior, span name construction
(default and INPUT_JOB_NAME), logging, and error propagation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the OTLP job-conclusion span sender to simplify its dependency loading and adds targeted unit tests around skip/call behavior, span naming, logging, and error propagation.
Changes:
- Simplified
action_conclusion_otlp.cjsto use a direct relativerequire("./send_otlp_span.cjs")and call via module reference. - Added
action_conclusion_otlp.test.cjswith unit tests using CJS export patching to mocksendJobConclusionSpan. - Expanded assertions around logging, span name construction, and error propagation.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/action_conclusion_otlp.cjs | Removes unused path logic and simplifies how send_otlp_span.cjs is referenced/called. |
| actions/setup/js/action_conclusion_otlp.test.cjs | Adds unit tests that mock the CJS dependency and validate skip/call/log/span-name/error behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/action_conclusion_otlp.cjs:32
send_otlp_span.cjsis now required at module load time, so it will be loaded even whenOTEL_EXPORTER_OTLP_ENDPOINTis unset andrun()early-returns. If you want to keep the skip path as lightweight as possible, consider moving therequire("./send_otlp_span.cjs")intorun()after the endpoint check while still calling it viamodule.sendJobConclusionSpan(...)(avoid destructuring) to preserve test patchability.
const sendOtlpSpan = require("./send_otlp_span.cjs");
/**
* Send the OTLP job-conclusion span. Non-fatal: all errors are silently
* swallowed.
- Files reviewed: 2/2 changed files
- Comments generated: 1
Comment on lines
+1
to
+5
| // @ts-check | ||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | ||
| import { createRequire } from "module"; | ||
|
|
||
| // Use CJS require so we share the same module cache as action_conclusion_otlp.cjs |
There was a problem hiding this comment.
PR description says there were "0 tests" before, but actions/setup/js/action_otlp.test.cjs already contains an action_conclusion_otlp run() test suite. Consider updating the PR description to reflect that this PR adds additional/targeted coverage rather than introducing the first tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleaned
action_conclusion_otlp.cjs— a Node.js context file that sends OTLP conclusion spans for GitHub Actions jobs.Context type
Node.js (not github-script) — uses
console.log,process.env, and CJSrequire.Changes made
action_conclusion_otlp.cjs:path = require("path")importrequire(path.join(__dirname, "send_otlp_span.cjs"))(dynamic path construction inside function) torequire("./send_otlp_span.cjs")at module top levelconst { sendJobConclusionSpan }) to module reference (const sendOtlpSpan = require(...)) so the dependency can be properly patched in tests — this is idiomatic CJS mock-patching styleaction_conclusion_otlp.test.cjs(new file — 11 tests):createRequire+ direct module-export patching to reliably mock CJS dependencies (avoids thevi.mock()/ native-require cache conflict)OTEL_EXPORTER_OTLP_ENDPOINTis not setsendJobConclusionSpanis NOT called when skippingsendJobConclusionSpanIS called when endpoint is setgh-aw.job.conclusion)INPUT_JOB_NAMEset (gh-aw.<name>.conclusion)sendJobConclusionSpanTest improvements
✅ All validation checks passed
npm run format:cjsnpm run lint:cjsnpm run typechecknpm run test:js -- --no-file-parallelism