Skip to content

fix: don't crash on tests with circular references in RuleTester #19664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

mdjermanovic
Copy link
Member

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 #19646

What changes did you make? (Give an overview)

Updated isSerializable (lib/shared/serialization.js) to not cause a stack overflow error when the passed object has circular references, and in that case return false because objects with circular references are not JSON-serializable.

Also updated freezeDeeply (in lib/rule-tester/rule-tester.js) to to not cause a stack overflow error when the passed object has circular references.

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

This is a very edge case, so I was more concerned if it would affect performance. By my performance testing, which was to run all core rules tests, it doesn't seem to significantly affect execution time.

Before:

$ time node node_modules/mocha/bin/_mocha -R min "tests/lib/rules/*.js"


  31860 passing (21s)


real    0m22.923s
user    0m0.015s 
sys     0m0.045s 

After:

$ time node node_modules/mocha/bin/_mocha -R min "tests/lib/rules/*.js"


  31860 passing (21s)


real    0m22.946s
user    0m0.000s 
sys     0m0.030s 

@mdjermanovic mdjermanovic requested a review from a team as a code owner April 27, 2025 17:23
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Apr 27, 2025
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Apr 27, 2025
@github-actions github-actions bot added the core Relates to ESLint's core APIs and features label Apr 27, 2025
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Apr 27, 2025
Copy link

netlify bot commented Apr 27, 2025

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 77fa69d
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/680e6827aaae460008433cc2

@mdjermanovic mdjermanovic moved this from Needs Triage to Implementing in Triage Apr 27, 2025
Copy link
Contributor

@snitin315 snitin315 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!

Leaving it open for 2nd review.

@snitin315 snitin315 moved this from Implementing to Second Review Needed in Triage Apr 28, 2025
Copy link
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.

LGTM. Thanks!

@nzakas nzakas merged commit d683aeb into main Apr 28, 2025
31 checks passed
@nzakas nzakas deleted the issue19646 branch April 28, 2025 14:47
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Apr 28, 2025
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 core Relates to ESLint's core APIs and features
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Bug: Stack overflow when serializing objects with circular references in eslint/lib/shared/serialization.js
3 participants