Skip to content

test: add tests for SuppressionsService.save() - #20802

Merged
DMartens merged 3 commits into
eslint:mainfrom
Kuldeep2822k:new
May 11, 2026
Merged

DMartens merged 3 commits into
eslint:mainfrom
Kuldeep2822k:new

Conversation

@Kuldeep2822k

Copy link
Copy Markdown
Contributor

Prerequisites checklist

AI acknowledgment

  • I did not use AI to generate this PR.
  • (If the above is not checked) I have reviewed the AI-generated content before submitting.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
Add unit tests for SuppressionsService.save().

What changes did you make? (Give an overview)

This is a follow-up to #20734 (load()), #20765 (suppress()), and #20797, continuing the incremental test coverage of SuppressionsService.

This PR adds a focused unit test for the save() method in lib/services/suppressions-service.js:

Test Case Code Path What It Verifies
Deterministic JSON formatting save() is called to write suppressions Verifies that writeFile is called with the correct file path, and that the data is valid JSON with stable alphabetical key sorting and 2-space indentation.

Uses sinon.stub on fs.promises.writeFile to fully isolate the method from the filesystem.

Is there anything you'd like reviewers to focus on?

The test verifies that the JSON stringification is deterministic (using json-stable-stringify-without-jsonify) by checking that the keys appear in the correct alphabetical order in the written string payload.

@Kuldeep2822k
Kuldeep2822k requested a review from a team as a code owner April 26, 2026 17:18
@eslint-github-bot eslint-github-bot Bot added the chore This change is not user-facing label Apr 26, 2026
@github-project-automation github-project-automation Bot moved this to Needs Triage in Triage Apr 26, 2026
@netlify

netlify Bot commented Apr 26, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit d72e8eb
🔍 Latest deploy log https://app.netlify.com/projects/docs-eslint/deploys/69f05bb763a84c0008c12200

Comment thread tests/lib/services/suppressions-service.js
@Kuldeep2822k

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! I've updated the PR to address this.

The save() test has now been split into two separate test cases for clarity and proper separation of concerns:

  1. should write valid JSON to filePath: Focuses on ensuring that writeFile is called with the correct path and verifying the written JSON strictly matches the provided data.
  2. should write deterministic JSON by sorting keys alphabetically and using 2-space indentation: Specifically verifies that the written JSON output is deterministic by checking that the keys are sorted alphabetically (also fixed a minor logic flaw with the index comparison to prevent false positives) and explicitly asserts the 2-space indentation requirement.

);

// Verify 2-space indentation is applied
assert.ok(

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.

I would see the 2-space indentation as an implementation detail which does not have to be tested.

@Kuldeep2822k
Kuldeep2822k requested a review from DMartens April 28, 2026 07:04
@nzakas nzakas moved this from Needs Triage to Triaging in Triage Apr 30, 2026
@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions Bot added the Stale label May 8, 2026

@DMartens DMartens 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.

Changes LGTM, thanks.

@DMartens
DMartens merged commit 24db8cb into eslint:main May 11, 2026
43 checks passed
@github-project-automation github-project-automation Bot moved this from Triaging to Complete in Triage May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore This change is not user-facing Stale

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants