Skip to content
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

Update to eslint v9 #221

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Update to eslint v9 #221

merged 1 commit into from
Jan 7, 2025

Conversation

ivy-lli
Copy link
Member

@ivy-lli ivy-lli commented Jan 6, 2025

No description provided.

@ivy-lli ivy-lli marked this pull request as ready for review January 6, 2025 13:01
@ivy-lli ivy-lli requested a review from ivy-lgi January 6, 2025 13:02
@@ -177,7 +178,7 @@ describe('removeNode', () => {
const originalData = structuredClone(data);
const newData = removeNode(data, [1, 1, 0]);
expect(data).toEqual(originalData);
expect(newData).not.toBe(data);
expect(newData).not.toEqual(data);
Copy link
Member

Choose a reason for hiding this comment

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

Why make this change in this specific test?

Some info about this line:
These functions under test are designed to not change the data provided as an argument directly. Instead, they create a clone, apply the changes to the clone, and then return the clone. This is important so an object held in state can be provided to the function without having to clone it beforehand.

This line asserts that the returned object of the function is a different one (by reference) than the object that was provided.

Although I think this line is redundant in many cases, I added it to every test just to be safe.

This cloning inside the function strategy is the reason for this whole pattern in every test:

const originalData = structuredClone(data); // make a copy of the original data for later use
const newData = removeNode(data, [1, 1, 0]); // call the function under test with the original data
expect(data).toEqual(originalData); // assert that the original data has not been changed
expect(newData).not.toEqual(data); // assert that the returned data is not equal by reference to the original data

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it because I tried to fix an eslint warning in the tree-data.
While change the function there I run this test failed, but not as I expected already on this line but on the following. toBe only checks that the references are not the same.
I think not.toEqual does the "same" as it also compares content of the two objects so if they don't have the same content they also don't have the same reference, but also fail if they have the same content (and the function didn't work)

But I can revert this change if you like.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine with me 👍

@ivy-lli ivy-lli merged commit ef96d7f into master Jan 7, 2025
10 checks passed
@ivy-lli ivy-lli deleted the eslint-v9 branch January 7, 2025 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants