Add a test case for onLocationChanged

[willdurand]

Fixes mozilla/addons#12502

---

This test case makes sure our helper function is always in sync with the
upstream lib.

[codecov-io]

Codecov Report

Merging #6595 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    mozilla/addons-frontend#6595   +/-   ##
=======================================
  Coverage   97.81%   97.81%           
=======================================
  Files         239      239           
  Lines        6512     6512           
  Branches     1239     1239           
=======================================
  Hits         6370     6370           
  Misses        127      127           
  Partials       15       15

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a233ba2...6e402c9. Read the comment docs.

[kumar303]

Thanks, I like the approach. I just have a minor change request.

[kumar303]

I think this should not use sinon.match() because we do want to know if there are more or less props in the payload. Was there a dynamic value giving you trouble? If so, you could create a custom sinon matcher.

[willdurand]

A "key" property gets generated automatically.

[kumar303]

Okay. Here's an example of a custom matcher:

addons-frontend/tests/unit/helpers.js

Lines 845 to 867 in 6dcad19

	/*
	* A sinon matcher to check if the URL contains the declared params.
	*
	* Example:
	*
	* mockWindow.expects('fetch').withArgs(urlWithTheseParams({ page: 1 }))
	*/
	export const urlWithTheseParams = (params) => {
	return sinon.match((urlString) => {
	const { query } = url.parse(urlString, true);
	for (const param in params) {
	if (
	query[param] === undefined ||
	query[param] !== params[param].toString()
	) {
	return false;
	}
	}
	return true;
	}, `urlWithTheseParams(${JSON.stringify(params)})`);
	};

[willdurand]

I see. It seems a lot of code to me, for a matcher that won't be reused.

The use of sinon.match here catches the following bugs:

• action set to another constant in the upstream lib
• location not set or with a different shape (i.e. pathname not present anymore)

That's why I thought that would be enough. Let me come back with something stricter yet still simple.

[kumar303]

🦑

[kumar303]

Ah, yes, I was thinking of sinon.match.string but I thought it only worked for mock expectations. I think this will be helpful.

[willdurand]

Yes, that's super interesting!