Skip to content

fix: switch away from Node.js node:assert and AssertionError#19082

Merged
mdjermanovic merged 6 commits intoeslint:mainfrom
JoshuaKGoldberg:internal-asserts
Nov 13, 2024
Merged

fix: switch away from Node.js node:assert and AssertionError#19082
mdjermanovic merged 6 commits intoeslint:mainfrom
JoshuaKGoldberg:internal-asserts

Conversation

@JoshuaKGoldberg
Copy link
Copy Markdown
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Oct 30, 2024

Prerequisites checklist

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

[ ] Documentation update
[x] 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
[ ] Other, please explain:

Fixes #19040.

What changes did you make? (Give an overview)

Switches from node:assert to equivalent hand-written function ok.

strictEqual and notStrictEqual are also used in the project, but only by the development-time RuleTester. We'd like to keep that as-is.

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

@eslint-github-bot eslint-github-bot Bot added the bug ESLint is working incorrectly label Oct 30, 2024
@github-actions github-actions Bot added cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features labels Oct 30, 2024
@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 30, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 679e63b
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6733c6a5e370a10008b34b2b

Comment thread lib/linter/report-translator.js Outdated
@JoshuaKGoldberg JoshuaKGoldberg changed the title fix: switch away from Node.js node:asserts and AssertionError fix: switch away from Node.js node:assert and AssertionError Oct 30, 2024
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review October 30, 2024 19:52
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team as a code owner October 30, 2024 19:52
@amareshsm amareshsm added the accepted There is consensus among the team that this change meets the criteria for inclusion label Oct 30, 2024
Comment thread lib/shared/assert.js Outdated
Comment thread lib/shared/assert.js Outdated
Comment thread lib/shared/assert.js Outdated
Comment thread lib/shared/assert.js Outdated
Copy link
Copy Markdown
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

I don't see any references to strictEqual or notStrictEqual. Did you forget to include some files with changes?

@mdjermanovic
Copy link
Copy Markdown
Member

We're using assert.strictEqual and assert.notStrictEqual in RuleTester. But I think it's good to keep using node:assert in RuleTester because it provides additional properties on errors, like expected and actual. Those properties are used for better output:

image

Other than RuleTester, I think we need just a simple assert(value, message).

JoshuaKGoldberg and others added 2 commits November 4, 2024 14:31
@JoshuaKGoldberg
Copy link
Copy Markdown
Contributor Author

Ah, yes, I'd reverted RuleTester to node:assert but not updated the rest. Thanks - updated now.

@JoshuaKGoldberg
Copy link
Copy Markdown
Contributor Author

Browser tests pass locally for me with node Makefile wdio.

Comment thread lib/shared/assert.js Outdated
@mdjermanovic
Copy link
Copy Markdown
Member

@JoshuaKGoldberg I think #19082 (comment) hasn't been addressed yet.

@JoshuaKGoldberg
Copy link
Copy Markdown
Contributor Author

Ah, sorry missed that - updated now.

Copy link
Copy Markdown
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I've verified that errors thrown for overlapping fixes now show the filename as requested in the original issue.

Oops! Something went wrong! :(

ESLint: 9.14.0

Error: Fix objects must not be overlapped in a report.
Occurred while linting C:\projects\eslint\Makefile.js:7
Rule: "no-multi-spaces"
    at ok (C:\projects\eslint\lib\shared\assert.js:17:15)
    at mergeFixes (C:\projects\eslint\lib\linter\report-translator.js:165:9)
    at normalizeFixes (C:\projects\eslint\lib\linter\report-translator.js:197:16)
    at C:\projects\eslint\lib\linter\report-translator.js:371:49
    at FileContext.report (C:\projects\eslint\lib\linter\linter.js:1048:41)
    at C:\projects\eslint\lib\rules\no-multi-spaces.js:129:29
    at Array.forEach (<anonymous>)
    at Program (C:\projects\eslint\lib\rules\no-multi-spaces.js:84:46)
    at ruleErrorHandler (C:\projects\eslint\lib\linter\linter.js:1084:48)
    at C:\projects\eslint\lib\linter\safe-emitter.js:45:58

@mdjermanovic mdjermanovic merged commit bd35098 into eslint:main Nov 13, 2024
@JoshuaKGoldberg JoshuaKGoldberg deleted the internal-asserts branch November 13, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

Change Request: Indicate the impacted file when AssertionError is thrown by report-translator.js

4 participants