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

expect: compare URLs by href (so that different URLs aren't considered equal) #4614

Closed
4 tasks done
kleinfreund opened this issue Nov 28, 2023 · 0 comments · Fixed by #4615
Closed
4 tasks done

expect: compare URLs by href (so that different URLs aren't considered equal) #4614

kleinfreund opened this issue Nov 28, 2023 · 0 comments · Fixed by #4615
Labels
enhancement New feature or request good first issue Good for newcomers p2-nice-to-have Not breaking anything but nice to have (priority) pr welcome

Comments

@kleinfreund
Copy link
Contributor

Clear and concise description of the problem

When comparing URLs in Vitest, tests easily pass for completely different URLs (in a sense, this can be considered a bug):

expect(new URL('https://example.org')).toEqual(new URL('https://different-example.org'))
// This passes, but it shouldn't:                               ^^^^^^^^^^

The main equals comparison utility can be easily updated to handle this case better. I'd be happy to contribute this.

Suggested solution

Add a special case for URL objects in https://github.com/vitest-dev/vitest/blob/main/packages/expect/src/jest-utils.ts#L96-L97:

    if (a instanceof Error && b instanceof Error)
      return a.message === b.message

+   if (a instanceof URL && b instanceof URL)
+     return a.href === b.href

And add test cases for this in https://github.com/vitest-dev/vitest/blob/main/test/core/test/jest-expect.test.ts#L51-L52:

      expect(new Date(0)).toEqual(new Date(0))
      expect(new Date('inValId')).toEqual(new Date('inValId'))

+     expect(new URL('https://example.org')).toEqual(new URL('https://example.org'))
+     expect(new URL('https://example.org')).not.toEqual(new URL('https://different-example.org'))

Alternative

Fail URL comparisons unless they're the exact same object.

Additional context

I initially noticed this behavior while working on a small wrapper around fetch where I have a lot of tests that very roughly look like this (though in place of fetch I'm calling my fetch wrapper, replaced here for brevity):

test('test', async () => {
	vi.spyOn(window, 'fetch').mockImplementation(() => Promise.resolve(new Response('OK')))

	await fetch(new URL('https://example.org'))

	expect(fetch).toHaveBeenCalledWith(new URL('https://different-example.org'))
})

I noticed that I couldn't get the assertion to fail at all. As a workaround, my tests actually look like this:

test('test', async () => {
	vi.spyOn(window, 'fetch').mockImplementation((input) => {
		expect(toUrl(input).href).toEqual(toUrl('https://different-example.org').href)

		return Promise.resolve(new Response('OK'))
	})

	await fetch(new URL('https://example.org'))

	expect(fetch).toHaveBeenCalled()
})

function toUrl(url: string | URL | Request): URL {
	return typeof url === 'string'
		? new URL(url)
		: url instanceof URL
			? url
			: new URL(url.url)
}

While this works great, it's really a little too cumbersome for my liking. Using toHaveBeenCalledWith would be much nicer for this use case.

Validations

@sheremet-va sheremet-va added enhancement New feature or request good first issue Good for newcomers pr welcome p2-nice-to-have Not breaking anything but nice to have (priority) labels Nov 28, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request good first issue Good for newcomers p2-nice-to-have Not breaking anything but nice to have (priority) pr welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants