test: test bottom-up merge sort in URLSearchParams

[watilde]

Following up for this commit: 02d1e32 to increase the reduced coverage. The bottom-up iterative stable merge sort is called only when the length of provided value is larger than 100. Added a test for it.

This increases the coverage of internal/url.js,

• https://coverage.nodejs.org/coverage-e4e7cd5bd202084b/root/internal/url.js.html

and the following lines will be covered.

• node/lib/internal/url.js

  Lines 701 to 727 in e4e7cd5

  	function merge(out, start, mid, end, lBuffer, rBuffer) {
  	const sizeLeft = mid - start;
  	const sizeRight = end - mid;
  	var l, r, o;
  	for (l = 0; l < sizeLeft; l++)
  	lBuffer[l] = out[start + l];
  	for (r = 0; r < sizeRight; r++)
  	rBuffer[r] = out[mid + r];
  	l = 0;
  	r = 0;
  	o = start;
  	while (l < sizeLeft && r < sizeRight) {
  	if (lBuffer[l] <= rBuffer[r]) {
  	out[o++] = lBuffer[l++];
  	out[o++] = lBuffer[l++];
  	} else {
  	out[o++] = rBuffer[r++];
  	out[o++] = rBuffer[r++];
  	}
  	}
  	while (l < sizeLeft)
  	out[o++] = lBuffer[l++];
  	while (r < sizeRight)
  	out[o++] = rBuffer[r++];
  	}

• node/lib/internal/url.js

  Lines 886 to 900 in e4e7cd5

  	} else {
  	// Bottom-up iterative stable merge sort
  	const lBuffer = new Array(len);
  	const rBuffer = new Array(len);
  	for (var step = 2; step < len; step *= 2) {
  	for (var start = 0; start < len - 2; start += 2 * step) {
  	var mid = start + step;
  	var end = mid + step;
  	end = end < len ? end : len;
  	if (mid > end)
  	continue;
  	merge(a, start, mid, end, lBuffer, rBuffer);
  	}
  	}
  	}

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

test

[joyeecheung]

If I am reading correctly the test part below just duplicates the code above? Then we can probably just construct the params first and then put it in the array literal that's being forEached above?

[watilde]

That's nice. I've updated it :)

[joyeecheung]

Hmm..so looks like this doesn't actually catch a mistake in the implementation because we are sorting identical pairs...maybe we can generate the keys on the fly, pseudo-shuffle them, then compared to another original copy to make sure they are sorted properly?

const tests = [{input: '', output: []}];
const pairs = [];
for (let i = 10; i < 60; i++) {
  pairs.push([`a${i}`, 'b']);
  tests[0].output.push([`a${i}`, 'b']);
}
tests[0].input = pairs.sort(() => Math.random() > 0.5).map((pair) => pair.join('=')).join('&');

[watilde]

That's right indeed. To use the random list to sort them actually, I've updated it with your comment. Thanks!

[TimothyGu]

LGTM.

[TimothyGu]

nit: I'd be more comfortable if there are more than just the threshold number of elements in pairs

[watilde]

It sounds good to me as well. I will replace 60 with 100 later :)

[watilde]

Ok, I've updated.

[TimothyGu]

CI: https://ci.nodejs.org/job/node-test-pull-request/6437/

[TimothyGu]

New CI: https://ci.nodejs.org/job/node-test-pull-request/6442/

[jasnell]

CI failures are unrelated.

[jasnell]

Landed in 1b31ca6