refactor: changelog test into js and add tests#232
Conversation
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
WalkthroughThe pull request replaces a bash-based changelog validation script with a Node.js implementation. It introduces a new Node.js validation script, comprehensive unit tests, and a GitHub Actions workflow to automatically validate changelog entries in pull requests. The old shell script is removed entirely. Changes
Sequence DiagramsequenceDiagram
participant GH as GitHub Actions
participant Git as Git Repository
participant Script as pr-check-primary-changelog.js
participant FS as File System
GH->>Git: Fetch full history (fetch-depth: 0)
GH->>Script: Execute with GITHUB_TOKEN
Script->>Git: Run git diff origin/{base}...HEAD -- CHANGELOG.md
Git-->>Script: Return diff output
Script->>FS: Read CHANGELOG.md
FS-->>Script: Return file contents
Script->>Script: Parse diff, extract added bullets
Script->>Script: Scan file for Unreleased section & subtitles
Script->>Script: Classify each added entry as valid/invalid
Script-->>GH: Exit with code 0 (valid) or 1 (invalid)
GH->>GH: Report pass/fail status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52b0e729-574d-46b0-a245-ea2df9485b55
📒 Files selected for processing (6)
.github/scripts/__tests__/pr-check-primary-changelog.test.js.github/scripts/pr-check-changelog.sh.github/scripts/pr-check-primary-changelog.js.github/workflows/pr-check-primary-changelog.yml.gitignoreCHANGELOG.md
💤 Files with no reviewable changes (1)
- .github/scripts/pr-check-changelog.sh
| @@ -0,0 +1,179 @@ | |||
| // .github/scripts/changelog-check.test.js | |||
There was a problem hiding this comment.
Incorrect filename in comment.
The comment says changelog-check.test.js but the file is named pr-check-primary-changelog.test.js.
📝 Suggested fix
-// .github/scripts/changelog-check.test.js
+// .github/scripts/__tests__/pr-check-primary-changelog.test.js📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // .github/scripts/changelog-check.test.js | |
| // .github/scripts/__tests__/pr-check-primary-changelog.test.js |
| // Load the REAL CHANGELOG.md so tests reflect actual structure | ||
| const base = fs.readFileSync("CHANGELOG.md", "utf-8").split("\n"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test assumes execution from repository root.
The path "CHANGELOG.md" is relative and assumes the test runs from the repository root. This is typically fine for CI but worth noting for local development. Consider using path.resolve(__dirname, '../../../CHANGELOG.md') for robustness.
| // Minimal test runner | ||
| function test(name, fn) { | ||
| try { | ||
| fn(); | ||
| console.log(`✅ ${name}`); | ||
| } catch (e) { | ||
| console.error(`❌ ${name}`); | ||
| console.error(" ", e.message); | ||
| process.exitCode = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests are not integrated into CI.
These tests are never executed by any workflow, which means they won't catch regressions. Consider adding a step to run them, either in this workflow or a dedicated test workflow.
♻️ Example: Add test step to the workflow
In .github/workflows/pr-check-primary-changelog.yml:
- name: Run changelog validation tests
run: node .github/scripts/__tests__/pr-check-primary-changelog.test.js
- name: Run local changelog check
run: node .github/scripts/pr-check-primary-changelog.jsAlternatively, consider using a test framework like Jest for better test reporting and parallel execution.
| // ❌ No changelog case: no entries added → validator should fail | ||
| test("no added entries", () => { | ||
| const lines = clone(); | ||
|
|
||
| const res = validateChangelog(lines, []); | ||
|
|
||
| expect(res.errors.length === 0, "should fail"); | ||
| }); |
There was a problem hiding this comment.
Test will crash and has confusing assertion.
Two issues:
-
This test will terminate the test runner because
validateChangelog([])callsprocess.exit(1)(line 14 of the main script) before returning, soresis never assigned. -
The assertion
expect(res.errors.length === 0, "should fail")is contradictory — it checks that errors is empty but the message says "should fail".
Once the process.exit is moved out of validateChangelog (as suggested in the main script review), update this test to verify the expected behavior.
🐛 Suggested fix (after main script is fixed)
// ❌ No changelog case: no entries added → validator should fail
-test("no added entries", () => {
+test("no added entries returns noEntries flag", () => {
const lines = clone();
const res = validateChangelog(lines, []);
- expect(res.errors.length === 0, "should fail");
+ expect(res.noEntries === true, "should indicate no entries were provided");
+ expect(res.errors.length === 0, "should have no errors array entries");
+ expect(res.valid.length === 0, "should have no valid array entries");
});| function validateChangelog(fileLines, addedLines) { | ||
| // Tracks whether we are currently inside the "Unreleased" section and subtitle | ||
| // Starts false or "" because we begin scanning from the top of the file. | ||
| if (addedLines.length === 0) { | ||
| console.log("❌ CHANGELOG.md was not updated in this PR"); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
process.exit(1) inside validateChangelog breaks testability and violates separation of concerns.
The validateChangelog function is exported for unit testing, but calling process.exit(1) directly inside it will terminate the test runner when testing the "no added entries" case. The test at line 151-157 in the test file will never complete properly.
Move the exit logic to runCI() and have validateChangelog return a result instead.
🐛 Proposed fix
function validateChangelog(fileLines, addedLines) {
- // Tracks whether we are currently inside the "Unreleased" section and subtitle
- // Starts false or "" because we begin scanning from the top of the file.
- if (addedLines.length === 0) {
- console.log("❌ CHANGELOG.md was not updated in this PR");
- process.exit(1);
- }
let inUnreleased = false;
let currentSubtitle = "";
// Collects entries that violate placement rules.
const errors = [];
// Collects entries that are correctly placed.
const valid = [];
+ // Flag for empty input
+ const noEntries = addedLines.length === 0;
+
+ if (noEntries) {
+ return { errors, valid, noEntries };
+ }
// Iterate through the entire changelog line-by-line.
// ... rest of function ...
// Return classification results for caller (CI or tests)
- return { errors, valid };
+ return { errors, valid, noEntries: false };
}Then in runCI():
const { errors, valid } = validateChangelog(fileLines, addedLines);
+
+ if (result.noEntries) {
+ console.log("❌ CHANGELOG.md was not updated in this PR");
+ process.exit(1);
+ }| try { | ||
| diff = execSync( | ||
| `git diff origin/${process.env.GITHUB_BASE_REF}...HEAD -- ${CHANGELOG}`, | ||
| { encoding: "utf-8" } | ||
| ); | ||
| } catch (e) { | ||
| console.error("❌ Failed to get git diff"); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Validate GITHUB_BASE_REF before use.
If GITHUB_BASE_REF is undefined (e.g., running locally without proper env setup), the git command becomes git diff origin/undefined...HEAD, producing a confusing error. As per coding guidelines, environment variables should be validated before use.
🛡️ Proposed fix
function runCI() {
+ const baseRef = process.env.GITHUB_BASE_REF;
+ if (!baseRef) {
+ console.error("❌ GITHUB_BASE_REF environment variable is not set");
+ process.exit(1);
+ }
// --- Extract PR diff for CHANGELOG.md ---
let diff;
try {
diff = execSync(
- `git diff origin/${process.env.GITHUB_BASE_REF}...HEAD -- ${CHANGELOG}`,
+ `git diff origin/${baseRef}...HEAD -- ${CHANGELOG}`,
{ encoding: "utf-8" }
);
} catch (e) {| if (addedLines.length === 0) { | ||
| console.log("⚠️ No changelog entries found in diff"); | ||
| process.exit(0); // assume earlier step enforces presence | ||
| } |
There was a problem hiding this comment.
Inconsistent handling of "no entries" case.
Here the script exits with 0 when no changelog entries are found, but validateChangelog (lines 12-15) exits with 1 for the same condition. This inconsistency creates confusing behavior:
- If the diff has no bullet entries → exit 0 (success)
- If
validateChangelogreceives empty array → exit 1 (failure)
Since the CI exits before reaching validateChangelog when addedLines is empty, the check in validateChangelog is dead code in the CI path but causes test failures.
Clarify the intended behavior and make it consistent.
| name: Primary PR Check - Changelog Validation | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, reopened, edited, synchronize] | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| changelog-check: | ||
| runs-on: ${{ (github.event.pull_request.head.repo.full_name == github.repository && github.repository_owner == 'hiero-ledger') && 'hl-sdk-py-lin-md' || 'ubuntu-latest' }} | ||
| steps: | ||
| - name: Harden the runner | ||
| uses: step-security/harden-runner@fe104658747b27e96e4f7e80cd0a94068e53901d # v2.16.1 | ||
| with: | ||
| egress-policy: audit | ||
|
|
||
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| with: | ||
| fetch-depth: 0 # Fetch full history to ensure we have the complete changelog context | ||
|
|
||
| - name: Run local changelog check | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: node .github/scripts/pr-check-primary-changelog.js |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if old workflow exists and references the bash script
echo "=== Checking old workflow ==="
if [ -f ".github/workflows/pr-check-changelog.yml" ]; then
echo "Old workflow exists:"
rg -n "pr-check-changelog.sh" .github/workflows/pr-check-changelog.yml
else
echo "Old workflow does not exist"
fi
echo ""
echo "=== Checking if bash script exists ==="
if [ -f ".github/scripts/pr-check-changelog.sh" ]; then
echo "Bash script still exists"
else
echo "Bash script has been deleted"
fiRepository: exploreriii/hiero_sdk_python
Length of output: 307
Delete or update the old workflow .github/workflows/pr-check-changelog.yml.
The old workflow still exists and references .github/scripts/pr-check-changelog.sh (lines 27–28), which has been deleted as part of this refactoring. This workflow will fail on every PR. Either delete the old workflow file or update it to call the new JavaScript script.
| - name: Run local changelog check | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: node .github/scripts/pr-check-primary-changelog.js |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
GITHUB_TOKEN is set but not used by the script.
The script only uses GITHUB_BASE_REF (auto-provided by GitHub Actions). The GITHUB_TOKEN environment variable is passed but never referenced in pr-check-primary-changelog.js.
♻️ Suggested simplification
- name: Run local changelog check
- env:
- GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: node .github/scripts/pr-check-primary-changelog.js📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run local changelog check | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| run: node .github/scripts/pr-check-primary-changelog.js | |
| - name: Run local changelog check | |
| run: node .github/scripts/pr-check-primary-changelog.js |
| .env/ | ||
| myenv/ | ||
|
|
||
| jest* |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which repository files are affected by the broad jest* pattern.
set -euo pipefail
echo "Rule in .gitignore:"
rg -n '^jest\*$' .gitignore || true
echo
echo "Files/directories starting with 'jest' in repo:"
fd -HI '^jest' . || true
echo
echo "Which of those are ignored by git:"
fd -HI '^jest' . | while read -r f; do
if git check-ignore -v "$f" >/tmp/check_ignore_out 2>/dev/null; then
echo "IGNORED: $f"
cat /tmp/check_ignore_out
fi
doneRepository: exploreriii/hiero_sdk_python
Length of output: 185
Consider using explicit Jest artifact paths in .gitignore instead of the broad jest* pattern.
The pattern jest* is overly broad and could unintentionally ignore legitimate configuration or helper files if added in the future. While no such files currently exist in the repository, using explicit paths is a safer practice for maintainability.
Suggested improvement
-jest*
+# Jest artifacts
+coverage/
+.jest-cache/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest* | |
| # Jest artifacts | |
| coverage/ | |
| .jest-cache/ |
1 participant
Converts changelog workflow into js
Changelog entry is correct under unreleased and a subheading
Should pass
Fixes #1