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

Consider alternatives to deepEqual #3476

Open
ptomato opened this issue Apr 11, 2022 · 1 comment
Open

Consider alternatives to deepEqual #3476

ptomato opened this issue Apr 11, 2022 · 1 comment

Comments

@ptomato
Copy link
Contributor

ptomato commented Apr 11, 2022

We briefly discussed in the maintainers' meeting of 2022-04-07, the deepEqual file in the harness has some problems in its semantics and we should at least encourage new tests not to use it, while working out a better way to provide that functionality.

I've taken a look at what uses it and identified three categories:

  1. Comparing two arrays with primitive values in them ­— it seems to me this can be replaced wholesale by compareArray, which is clearer and more fit for purpose anyway.
  2. Comparing two objects with primitive values as their keys, where order of assignment doesn't does matter.
  3. Comparing two arrays with non-primitive values in them.

Here are the places where it's currently used:

Providing alternatives for the remaining uses could be an interesting first test of our RFC process once I have come up with a proposal for that.

@ptomato
Copy link
Contributor Author

ptomato commented Apr 11, 2022

And then, some random ideas that I have so far:

For (3), maybe it could be replaced by a compareArrayElementsWith helper that takes the two arrays and applies a supplied comparison function to each element, kind of like is done already elsewhere in formatToParts: here

Taking the above link as an example, I'd imagine an API that might look like this if that file were rewritten to use it:

function compareFormatRangePart(actual, expected, index, description) {
  assert.sameValue(actual.type, expected.type, `${description}: type for part ${index}`);
  assert.sameValue(actual.value, expected.value, `${description}: value for part ${index}`);
  assert.sameValue(actual.source, expected.source, `${description}: source for part ${index}`);
}

assert.compareArrayElementsWith(
  compareFormatRangePart,
  dtf.formatRangeToParts(d1, d2),
  [
    { type: "minute", value: "02", source: "shared" },
    { type: "literal", value: ":", source: "shared" },
    { type: "second", value: "03", source: "shared" },
  ],
  "dates with same minutes and seconds"
);

For (2), a similar compareObjectEntriesWith() helper, although ideally the name should make it clear that the order of assignment matters.

ptomato added a commit to ptomato/test262 that referenced this issue Oct 19, 2022
Using assert.deepEqual was faulty here, since deepEqual doesn't take
symbol keys into account. This test wasn't actually testing that the
symbol keys were absent, and in fact passes if they are present.

(Rather than fixing deepEqual, since we hope to deprecate it as per
tc39#3476, add a custom assert function
in the test.)
ptomato added a commit to ptomato/test262 that referenced this issue Oct 19, 2022
Using assert.deepEqual was faulty here, since deepEqual doesn't take
symbol keys into account. This test wasn't actually testing that the
symbol keys were absent, and in fact passes if they are present.

(Rather than fixing deepEqual, since we hope to deprecate it as per
tc39#3476, add a custom assert function
in the test.)
ptomato added a commit to ptomato/test262 that referenced this issue Oct 20, 2022
Using assert.deepEqual was faulty here, since deepEqual doesn't take
symbol keys into account. This test wasn't actually testing that the
symbol keys were absent, and in fact passes if they are present.

(Rather than fixing deepEqual, since we hope to deprecate it as per
tc39#3476, add a custom assert function
in the test.)
Ms2ger pushed a commit to ptomato/test262 that referenced this issue Oct 20, 2022
Using assert.deepEqual was faulty here, since deepEqual doesn't take
symbol keys into account. This test wasn't actually testing that the
symbol keys were absent, and in fact passes if they are present.

(Rather than fixing deepEqual, since we hope to deprecate it as per
tc39#3476, add a custom assert function
in the test.)
Ms2ger pushed a commit that referenced this issue Oct 20, 2022
Using assert.deepEqual was faulty here, since deepEqual doesn't take
symbol keys into account. This test wasn't actually testing that the
symbol keys were absent, and in fact passes if they are present.

(Rather than fixing deepEqual, since we hope to deprecate it as per
#3476, add a custom assert function
in the test.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant