harness/deepEqual.js: Improve formatting and align with base assert

[gibson042]

Consolidates handling of identity-bearing values via a lazy multi-use indication.

comparison

const v = [];
v.push(v);
const values = [
  '',
  Symbol('foo'),
  /./gui,
  [-0, 0, 0n],
  ((a, b) => [a, b, a, b, {}])({}, {}),
  new Date(0),
  { [Symbol.toStringTag]: 'errors', error: Error(), typeError: TypeError('foo') },
  function fn(){},
  new Set([v, {}, {}]),
  Object.create(null),
];
for (const x of values) v.push(x, x, 'NL');
String(assert.deepEqual.format(v)).replace(/ "NL",/g, '\n');

before

Note the frequently missing "as #N" and the total lack of indication for referentially-identical symbols/regular expressions/dates/etc., not to mention the inability to differentiate signed zeros from each other or numbers from bigints.

[[Ref: #1], "", "",
 Symbol(foo), Symbol(foo),
 /./giu, /./giu,
 [0, 0, 0], [Ref: #2],
 [Object {  }, Object {  }, [Ref: #4], [Ref: #5], Object {  }], [Ref: #3],
 Date "1970-01-01T00:00:00.000Z", Date "1970-01-01T00:00:00.000Z",
 errors { error: Object {  }, typeError: Object {  } }, [Ref: #7],
 [Function: fn], [Function: fn],
 Set {[Ref: #1], Object {  }, Object {  }}, [Ref: #10],
 [Object: null prototype] {  }, [Ref: #13], "NL"] as #1

after

[ref #1, "", "",
 Symbol(foo) as #2, ref #2,
 /./giu as #3, ref #3,
 [-0, 0, 0n] as #4, ref #4,
 [Object {} as #6, Object {} as #7, ref #6, ref #7, Object {}] as #5, ref #5,
 Date("1970-01-01T00:00:00.000Z") as #9, ref #9,
 errors {error: error Error(""), typeError: error TypeError("foo")} as #10, ref #10,
 function fn as #13, ref #13,
 Set {ref #1, Object {}, Object {}} as #14, ref #14,
 [Object: null prototype] {} as #17, ref #17, "NL"] as #1

[ptomato]

I had a look at the code, but before I figure out how the template tag works, a before/after comparison of the output would be helpful, as well as a bit more explanation of the motivation 😄

By the way, is #3476 on your radar? I've been considering the use of deepEqual in new tests discouraged.

[gibson042]

before I figure out how the template tag works

Ah, I wonder if I should try to add some inline documentation. But basically, lazyStringFactory`…` returns a function that must be called with an argument for each substitution value in the template and returns an object with only a toString method. Calling that method (i.e., trying to use the returned object as a string as in assert.deepEqual) passes each substitution value to its mapper and uses the results for simple interpolation. The reason for this is that the usage objects are expected to be updated as object graph traversal that follows formatting of one value discovers reuse of the value being formatted, so we want to delay rendering of those usage objects until after the outermost format call has returned.

a before/after comparison of the output would be helpful

Sure; added above.

as well as a bit more explanation of the motivation

It started as fixing issues like the extra closing brace in formatted map output and handling -0 to align with assert._toString (for e.g. assert.sameValue), but grew a little bit to better present my results for tc39/ecma262#3388 (in particular fixing the opacity of error instances):

// { "byteLength": "Symbol(Symbol.toPrimitive) returns too big for index", "maxByteLength": "getter throws", "newTarget": "get prototype throws" }
 expected  [["byteLength", "get", Symbol(Symbol.toPrimitive) as #3], ["byteLength", ref #3, "call"], ErrorShape {ctor: function RangeError}]
   actual  [["byteLength", "get", Symbol(Symbol.toPrimitive) as #3], ["byteLength", ref #3, "call"], ["options", "get", "maxByteLength"], error GetMaxByteLengthError("")]

By the way, is #3476 on your radar? I've been considering the use of deepEqual in new tests discouraged.

Hmm, it really does feel like the right tool for comparing logs as above.

[Ms2ger]

Seems okay to me

[Ms2ger]

Do we have a policy on if/else if after return? I personally prefer if, but will accept house rules

[gibson042]

Changed back to that style; else if ended up too dense to be readable.

[Ms2ger]

Please do add some docs here

[gibson042]

Done, with corresponding refactoring.

[ljharb]

in general, let's please avoid newer syntax unless it's required for the test (including spread, arrows, let/const, etc)

using tagged template literals also seems like it will permanently prevent the tests from running on an engine that doesn't have template literal support. is there a way to achieve the same goal without using backticks?

[ljharb]

Suggested change

	let join = arr => arr.join(', ');
	function join(arr) { return arr.join(', '); }

[ljharb]

Suggested change

	let getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;
	var getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;

[gibson042]

in general, let's please avoid newer syntax unless it's required for the test (including spread, arrows, let/const, etc)

using tagged template literals also seems like it will permanently prevent the tests from running on an engine that doesn't have template literal support. is there a way to achieve the same goal without using backticks?

I very much agree with this, but deepEqual.js already uses templates, arrows, and lexical bindings:

test262/harness/deepEqual.js

Lines 59 to 63 in 5ae7de9

	let tag = Symbol.toStringTag in value ? value[Symbol.toStringTag] : 'Object';
	if (tag === 'Object' && Object.getPrototypeOf(value) === null) {
	tag = '[Object: null prototype]';
	}
	return `${tag ? `${tag} ` : ''}{ ${Object.keys(value).map(key => `${key.toString()}: ${assert.deepEqual.format(value[key], seen)}`).join(', ')} }${usage.used ? ` as #${usage.id}` : ''}`;

A somewhat ergonomic fix printf-style fix is possible, but really seems more appropriate for a followup:

var tag = Symbol.toStringTag && Symbol.toStringTag in value
  ? value[Symbol.toStringTag]
  : Object.getPrototypeOf(value) === null ? '[Object: null prototype]' : 'Object';
let keys = Reflect.ownKeys(value).filter(function(key) {
  return getOwnPropertyDescriptor(value, key).enumerable;
});
let contents = keys.map(function(key) {
  // return lazyString`${escapeKey(key)}: ${format(value[key], seen)}`;
  return callTag(lazyString, '%v: %v', escapeKey(key), format(value[key], seen));
});
// return lazyResult`${tag ? `${tag} ` : ''}{${contents}}`(String, join);
return callTag(lazyResult, '%v{%v}', tag ? String(tag) + ' ' : '', contents)(String, join);

[ljharb]

Indeed if the syntax is already in the file, my comment doesn't apply.