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
Remove all assert_object_equals instances from the suite. #2033
Comments
Still waiting on reviews for the two PRs linked above. Anyone? @jgraham ? |
@inexorabletash you wanna take care of the |
@sideshowbarker Hrm, I'm not seeing one. https://github.com/w3c/web-platform-tests/search?utf8=%E2%9C%93&q=assert_object_equals |
Sorry, yeah, the URL I pasted in before isn’t for the file I actually found it in locally. The one I found it in locally is So never mind about that one :-) |
We still have: |
@jgraham: Is
And confusingly, At this point, would it be easier to fix what's wrong with |
@jgraham, is the discussion in https://critic.hoppipolla.co.uk/showcomment?chain=12186 lost forever? |
See also #20844. It seems like |
Reviewing usage, I note the following patterns in descending order of occurrence:
assert_object_equals(actual, {prop1: value1, prop2: value2, ... propN: valueN});
// rewrite to:
assert_equals(actual.prop1, expected.prop1);
assert_equals(actual.prop2, expected.prop2);
assert_equals(actual.propN, expected.propN);
assert_object_equals(actual, {prop1: value1, prop2: value2, ... propN: valueN});
// rewrite to:
assert_equals(Object.keys(actual).length, N);
assert_equals(actual.prop1, expected.prop1);
assert_equals(actual.prop2, expected.prop2);
assert_equals(actual.propN, expected.propN);
Given the prevalence of the first two, I'd suggest adding a new simpler function to testharness.js, e.g. |
Does |
I think those deep-compare cases could probably have a stricter set of assertions in many cases, e.g. after a clone, should be plain-old-objects and own-properties in most cases. But I didn't spend a huge amount of time looking at them to classify them further. I worry about having a generic |
FYI I had started classifying these in this spreadsheet. I see that @inexorabletash did a much better job of this than me though really, in that they pulled out some proper patterns! |
@stephenmcgruer - hey, nice sheet! More detailed than what I was doing. Since you've gone through the calls... what's your take on what we should do? |
This test was converted from js-test.js to testharness.js in https://crrev.com/625734 and incorrectly used assert_object_equals. * assert_object_equals() will assert that enumerable properties are the same. Since `new Number(42)` doesn't have any own properties, nothing useful was actually being asserted! * assert_equals() is sufficient for an object identity test. * The original test used `new Number(42)` to mint an object that would serialize as something distinct. This isn't necessary with testharness.js assertions - using {} is good enough. assert_object_equals() has lots of non-intuitive behavior, see #2033 for more. Change-Id: I202f27c228c7ec65407d9eae724af21181e6e684
This test was converted from js-test.js to testharness.js in https://crrev.com/625734 and incorrectly used assert_object_equals. * assert_object_equals() will assert that enumerable properties are the same. Since `new Number(42)` doesn't have any own properties, nothing useful was actually being asserted! * assert_equals() is sufficient for an object identity test. * The original test used `new Number(42)` to mint an object that would serialize as something distinct. This isn't necessary with testharness.js assertions - using {} is good enough. assert_object_equals() has lots of non-intuitive behavior, see #2033 for more. Change-Id: I202f27c228c7ec65407d9eae724af21181e6e684 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067565 Auto-Submit: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743523}
This test was converted from js-test.js to testharness.js in https://crrev.com/625734 and incorrectly used assert_object_equals. * assert_object_equals() will assert that enumerable properties are the same. Since `new Number(42)` doesn't have any own properties, nothing useful was actually being asserted! * assert_equals() is sufficient for an object identity test. * The original test used `new Number(42)` to mint an object that would serialize as something distinct. This isn't necessary with testharness.js assertions - using {} is good enough. assert_object_equals() has lots of non-intuitive behavior, see web-platform-tests/wpt#2033 for more. Change-Id: I202f27c228c7ec65407d9eae724af21181e6e684 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067565 Auto-Submit: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743523}
This test was converted from js-test.js to testharness.js in https://crrev.com/625734 and incorrectly used assert_object_equals. * assert_object_equals() will assert that enumerable properties are the same. Since `new Number(42)` doesn't have any own properties, nothing useful was actually being asserted! * assert_equals() is sufficient for an object identity test. * The original test used `new Number(42)` to mint an object that would serialize as something distinct. This isn't necessary with testharness.js assertions - using {} is good enough. assert_object_equals() has lots of non-intuitive behavior, see #2033 for more. Change-Id: I202f27c228c7ec65407d9eae724af21181e6e684 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067565 Auto-Submit: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743523}
This test was using assert_object_equals() to verify object identity, similar to #20844 which (a) wasn't validating identity and (2) failing in all browsers because that assert function is subtle/weird. Switch to just assert_equals() and remove expectation file. General issue: #2033 Change-Id: I9ee44d251b53955bb9101810142aac8e89a8b5be
This test was using assert_object_equals() to verify object identity, similar to #20844 which (a) wasn't validating identity and (2) failing in all browsers because that assert function is subtle/weird. Switch to just assert_equals() and remove expectation file. General issue: #2033 Change-Id: I9ee44d251b53955bb9101810142aac8e89a8b5be Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068248 Commit-Queue: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Auto-Submit: Joshua Bell <jsbell@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743639}
This test was using assert_object_equals() to verify object identity, similar to #20844 which (a) wasn't validating identity and (2) failing in all browsers because that assert function is subtle/weird. Switch to just assert_equals() and remove expectation file. General issue: #2033 Change-Id: I9ee44d251b53955bb9101810142aac8e89a8b5be Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068248 Commit-Queue: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Auto-Submit: Joshua Bell <jsbell@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743639}
This test was using assert_object_equals() to verify object identity, similar to web-platform-tests/wpt#20844 which (a) wasn't validating identity and (2) failing in all browsers because that assert function is subtle/weird. Switch to just assert_equals() and remove expectation file. General issue: web-platform-tests/wpt#2033 Change-Id: I9ee44d251b53955bb9101810142aac8e89a8b5be Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068248 Commit-Queue: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Auto-Submit: Joshua Bell <jsbell@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743639}
…equals, a=testonly Automatic update from web-platform-tests WPT: Fix incorrect use of assert_object_equals This test was using assert_object_equals() to verify object identity, similar to web-platform-tests/wpt#20844 which (a) wasn't validating identity and (2) failing in all browsers because that assert function is subtle/weird. Switch to just assert_equals() and remove expectation file. General issue: web-platform-tests/wpt#2033 Change-Id: I9ee44d251b53955bb9101810142aac8e89a8b5be Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068248 Commit-Queue: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Auto-Submit: Joshua Bell <jsbell@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743639} -- wpt-commits: 06000cecb5cf248e8edc9e73ccb71b0ce8f09c44 wpt-pr: 21921
…equals, a=testonly Automatic update from web-platform-tests WPT: Fix incorrect use of assert_object_equals This test was using assert_object_equals() to verify object identity, similar to web-platform-tests/wpt#20844 which (a) wasn't validating identity and (2) failing in all browsers because that assert function is subtle/weird. Switch to just assert_equals() and remove expectation file. General issue: web-platform-tests/wpt#2033 Change-Id: I9ee44d251b53955bb9101810142aac8e89a8b5be Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068248 Commit-Queue: Joshua Bell <jsbellchromium.org> Commit-Queue: Stephen McGruer <smcgruerchromium.org> Auto-Submit: Joshua Bell <jsbellchromium.org> Reviewed-by: Stephen McGruer <smcgruerchromium.org> Cr-Commit-Position: refs/heads/master{#743639} -- wpt-commits: 06000cecb5cf248e8edc9e73ccb71b0ce8f09c44 wpt-pr: 21921 UltraBlame original commit: 8f7e2091913062dbc95ab0bdbf0e9b02cee04d49
…equals, a=testonly Automatic update from web-platform-tests WPT: Fix incorrect use of assert_object_equals This test was using assert_object_equals() to verify object identity, similar to web-platform-tests/wpt#20844 which (a) wasn't validating identity and (2) failing in all browsers because that assert function is subtle/weird. Switch to just assert_equals() and remove expectation file. General issue: web-platform-tests/wpt#2033 Change-Id: I9ee44d251b53955bb9101810142aac8e89a8b5be Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068248 Commit-Queue: Joshua Bell <jsbell@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Auto-Submit: Joshua Bell <jsbell@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#743639} -- wpt-commits: 06000cecb5cf248e8edc9e73ccb71b0ce8f09c44 wpt-pr: 21921
https://web.archive.org/web/20160402125934/https://critic.hoppipolla.co.uk/showcomment?chain=12186 has it, edited OP. |
@stephenmcgruer pointed out to me we wanted to get rid of assert_object_equals in #25083, which I'd totally forgotten about. https://web-platform-tests.org/writing-tests/testharness-api.html doesn't give it as deprecated, either. |
…, a=testonly Automatic update from web-platform-tests Docs: assert_object_equals is DEPRECATED (#25481) See web-platform-tests/wpt#2033. -- wpt-commits: 506aebf28c1ae528835241a32d0a2fb3db4e6dfc wpt-pr: 25481
…, a=testonly Automatic update from web-platform-tests Docs: assert_object_equals is DEPRECATED (#25481) See web-platform-tests/wpt#2033. -- wpt-commits: 506aebf28c1ae528835241a32d0a2fb3db4e6dfc wpt-pr: 25481
…, a=testonly Automatic update from web-platform-tests Docs: assert_object_equals is DEPRECATED (#25481) See web-platform-tests/wpt#2033. -- wpt-commits: 506aebf28c1ae528835241a32d0a2fb3db4e6dfc wpt-pr: 25481 UltraBlame original commit: 0e68a2763626103b288864cdf0236008fe31acf8
…, a=testonly Automatic update from web-platform-tests Docs: assert_object_equals is DEPRECATED (#25481) See web-platform-tests/wpt#2033. -- wpt-commits: 506aebf28c1ae528835241a32d0a2fb3db4e6dfc wpt-pr: 25481 UltraBlame original commit: 0e68a2763626103b288864cdf0236008fe31acf8
…, a=testonly Automatic update from web-platform-tests Docs: assert_object_equals is DEPRECATED (#25481) See web-platform-tests/wpt#2033. -- wpt-commits: 506aebf28c1ae528835241a32d0a2fb3db4e6dfc wpt-pr: 25481 UltraBlame original commit: 0e68a2763626103b288864cdf0236008fe31acf8
See https://critic.hoppipolla.co.uk/showcomment?chain=12186 and #2030 (comment).
@jgraham @inexorabletash
[critic thread added from archive.org:]
@jgraham posted 2015-07-13 21:50 +00:00
@inexorabletash posted 2015-07-14 16:31 +00:00
@inexorabletash posted 2015-07-15 23:03 +00:00
The text was updated successfully, but these errors were encountered: