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

fix: comparing different URL objects now return false #14398

Closed
wants to merge 1 commit into from
Closed

fix: comparing different URL objects now return false #14398

wants to merge 1 commit into from

Conversation

tr1ckydev
Copy link
Contributor

Summary

Previously, jest would report this test as PASS.

test('url test', () => {
    expect(new URL("https://jestjs.io/")).toEqual(new URL("https://jestjs.io/docs/getting-started"));
});

In this pull request, this gets fixed and jest properly compares the two URL objects and reports FAIL.

Test plan

test('url test', () => {
    expect(new URL("https://jestjs.io/")).toEqual(new URL("https://jestjs.io/"));
});
// Results in PASS.
test('hash test', () => {
    expect(new URL("https://jestjs.io/docs/getting-started")).toEqual(new URL("https://jestjs.io/docs/getting-started#using-babel"));
});
// Results in FAIL.

@linux-foundation-easycla
Copy link

CLA Not Signed

@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2a917c5
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/64d4a18ac96afe00084158d0
😎 Deploy Preview https://deploy-preview-14398--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mrazauskas
Copy link
Contributor

mrazauskas commented Aug 10, 2023

In Node test environment everything works as you expect. This is only an issue with Jsdom. Better open an issue, because it can be that Jest is not the cause of this problem.

@mrazauskas
Copy link
Contributor

Seems like you are hitting #13936. That issue got missed and locked unfortunately. Just open a new ticket.

@tr1ckydev tr1ckydev closed this by deleting the head repository Sep 18, 2023
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2023
@jestjs jestjs unlocked this conversation Nov 2, 2023
@SimenB
Copy link
Member

SimenB commented Nov 2, 2023

Even if we look at symbol properties, I think we can have a fast path that just checks the href like this PR proposed.

@tr1ckydev would you be willing to restore your fork?

@tr1ckydev
Copy link
Contributor Author

@SimenB Sure, should I open a new PR?

@SimenB
Copy link
Member

SimenB commented Nov 2, 2023

Yeah, unless this one just reopens 🙂

@tr1ckydev
Copy link
Contributor Author

Done #14672

Copy link

github-actions bot commented Dec 3, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants