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

Some unit tests don't make sense #108

Open
j-refs opened this issue Jan 30, 2024 · 5 comments
Open

Some unit tests don't make sense #108

j-refs opened this issue Jan 30, 2024 · 5 comments

Comments

@j-refs
Copy link

j-refs commented Jan 30, 2024

Trawling thru the unit tests, I've noticed some failed tests that I really think should pass.
For example:
not ok 223 empty buffer and empty array are not equal
I, would honestly expect and empty buffer and empty array to NOT be equal. One holds arbitrary data and one holds bytes. So why then, are them not being equal a fail?
Also:
not ok 303 Date and RegExp are not equal
I cannot see any scenario where a Date and a RegExp would be any type of equal, them being 2 completely different types with completely different intended functionality. So then why is them not being equal a failed test?

I want to adopt this library, but I need to ensure I won't have any... Surprises.

Can someone explain these test results and maybe convince me to integrate this library?

@ljharb
Copy link
Member

ljharb commented Jan 30, 2024

I'm confused, all the tests are passing on main.

As for buffers and arrays, they are considered not equal, whether strict or loose, regardless of contents.

Similarly, Dates and RegExps are not equal, whether strict or loose.

In what node/npm version, or browser, are you running into these failing tests?

@j-refs
Copy link
Author

j-refs commented Feb 25, 2024

I was simply referencing your CI tests

@j-refs
Copy link
Author

j-refs commented Feb 25, 2024

The one I was referring to, regarding your program determining that a date and RegExp are equal, based on your test:
https://github.com/inspect-js/node-deep-equal/actions/runs/7715085317/job/21028804846#step:4:537

@j-refs
Copy link
Author

j-refs commented Feb 25, 2024

Just saying, some of those test results are very concerning from a long term stability viewpoint. I'd love to use this library, but I wouldn't feel good about code considering a date and a RegExp the same being in stability focused code. So I'd like to know what that's all about so I can possibly reconsider, because this looks more promising than anything else

@ljharb
Copy link
Member

ljharb commented Feb 25, 2024

Those are the native node tests, that don’t invoke this package’s implementation. It’s in node < 12 that that’s the case.

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

No branches or pull requests

2 participants