Skip to content

[jsweep] Clean action_conclusion_otlp.cjs - #31420

Merged
pelikhan merged 1 commit into
mainfrom
jsweep/action-conclusion-otlp-61ad360fe7a85bf2
May 11, 2026
Merged

[jsweep] Clean action_conclusion_otlp.cjs#31420
pelikhan merged 1 commit into
mainfrom
jsweep/action-conclusion-otlp-61ad360fe7a85bf2

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

jsweep: Clean action_conclusion_otlp.cjs

Context: Pure Node.js module (invoked from post.js and clean.sh)

Changes

  • Number() + Number.isFinite() instead of parseInt: More explicit about rejecting NaN and Infinity when parsing GITHUB_AW_OTEL_JOB_START_MS. parseInt('Infinity', 10) returns NaN (falsy, same result), but the new code documents intent clearly via Number.isFinite.
  • Reordered logic: spanName/startMs are now computed before the endpoint branch — reads top-to-bottom without jumping.
  • Positive-condition-first: Changed if (!endpoints) { ... } else { ... } to if (endpoints) { ... } else { ... } to remove the double-negation cognitive load.

Tests

  • Before: 17 tests
  • After: 18 tests (+1)
  • New test: should pass startMs: undefined when GITHUB_AW_OTEL_JOB_START_MS is Infinity — covers the Number.isFinite guard that parseInt did not explicitly test.

✅ Validation

  • Formatting: npm run format:cjs
  • Linting: npm run lint:cjs
  • Type checking: npm run typecheck
  • Tests: npx vitest run action_conclusion_otlp.test.cjs — 18/18 passed ✓

Generated by jsweep - JavaScript Unbloater · ● 10.9M ·

  • expires on May 13, 2026, 5:04 AM UTC

- Use Number() + Number.isFinite() instead of parseInt for clearer NaN/Infinity handling
- Reorder spanName/startMs computation before endpoint branch
- Simplify if/else to positive-condition-first (remove double negation)
- Add test for Infinity input (startMs: undefined path via Number.isFinite)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@pelikhan
pelikhan marked this pull request as ready for review May 11, 2026 06:06
CopilotAI review requested due to automatic review settings May 11, 2026 06:06

CopilotAI 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.

Pull request overview

Cleans up action_conclusion_otlp.cjs to make the startMs parsing more explicit and the control flow easier to read, and adds a targeted test to cover the new parsing guard.

Changes:

  • Switches GITHUB_AW_OTEL_JOB_START_MS parsing to Number() with a Number.isFinite() guard (rejects NaN / Infinity explicitly).
  • Reorders spanName / startMs computation ahead of the endpoint logging branch and flips the branch to a positive check (if (endpoints)).
  • Adds a new test case ensuring "Infinity" results in startMs: undefined.
Show a summary per file
FileDescription
actions/setup/js/action_conclusion_otlp.cjsRefactors startMs parsing and slightly restructures endpoint-logging flow for clarity.
actions/setup/js/action_conclusion_otlp.test.cjsAdds test coverage for the "Infinity" start timestamp case.

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

@github-actionsgithub-actionsBot mentioned this pull request May 11, 2026

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 1.6M

@github-actions

Copy link
Copy Markdown
ContributorAuthor

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100

Excellent test quality

MetricValue
New/modified tests analyzed1
✅ Design tests (behavioral contracts)1 (100%)
⚠️ Implementation tests (low value)0 (0%)
Tests with error/edge cases1 (100%)
Duplicate test clusters0
Test inflation detectedNo (8 test lines / 8 prod lines = 1:1)
🚨 Coding-guideline violationsNone

Test Classification Details

TestFileClassificationIssues Detected
should pass startMs: undefined when GITHUB_AW_OTEL_JOB_START_MS is Infinityactions/setup/js/action_conclusion_otlp.test.cjs✅ DesignNone

No Flagged Tests

All analyzed tests are classified as design tests with no issues detected.


Language Support

Tests analyzed:

  • 🟨 JavaScript (*.test.cjs): 1 test (vitest)

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%).

The new test directly validates the behavioral contract introduced in the production change: when GITHUB_AW_OTEL_JOB_START_MS is set to "Infinity", Number("Infinity") yields a non-finite value, so startMs must be coerced to undefined rather than forwarded to the span. This is a genuine edge-case behavioral test that would catch a regression if the Number.isFinite() guard were removed.

The mock target (sendJobConclusionSpan) is external telemetry I/O — an acceptable mocking target per project guidelines.

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References:§25653320848

🧪 Test quality analysis by Test Quality Sentinel · ● 4.8M ·

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%).

@pelikhan
pelikhan merged commit 65faa04 into mainMay 11, 2026
34 checks passed
@pelikhan
pelikhan deleted the jsweep/action-conclusion-otlp-61ad360fe7a85bf2 branch May 11, 2026 10:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pelikhan