Remove all assert_object_equals instances from the suite.

sideshowbarker commented 9 years ago

See https://critic.hoppipolla.co.uk/showcomment?chain=12186 and https://github.com/w3c/web-platform-tests/pull/2030#issue-96433741. @jgraham @inexorabletash

[critic thread added from archive.org:]

@jgraham posted 2015-07-13 21:50 +00:00

So I really really wish this function didn't exist; I think it's almost never a good idea to use it. This change is also backward-incompatible although hopefully no one is abusing the current behaviour.

I would honestly prefer to eradicate all uses of this function. What's the use case you have in mind?

@inexorabletash posted 2015-07-14 16:31 +00:00

The motiviation for this change is its use with the Service Worker Cache Storage tests, which put a Request/Response pair in and validate that the properties are the same when retrieved.

Blink was monkey-patching assert_object_equals() with something that worked but was a very different assertion and buggy in a different way. I recently eliminated the monkey-patch (well, at least one, just found another) and wanted to just use the built-in, but properly fixed.

I've been looking at Blinks (pre-upstream) copies of the tests. The upstreamed ones still have the monkey patch: https://github.com/w3c/web-platform-tests/blob/master/service-workers/cache-storage/resources/testharness-helpers.js (which is buggy and a bad idea anyway)

But in the abstract, the use case is to take two DOM objects and assert that "properties" are equivalent. We could introduce a new assertion for that, or write a type specific helper just for the SW/CS tests.

@inexorabletash posted 2015-07-15 23:03 +00:00

It looks like the function has repeated use in:

IndexedDB/ - comparing keys, values, arrays of strings (i.e. plain JS objects) service-workers/cache-storage/ - as noted, comparing Requests/Responses (DOM objects); could use dedicated comparison functions webmessaging/ - comparing cloned objects, also has a wrapper handles Date and RegExp instances

And then a couple of one-offs:

ext-xhtml-pubid/the-xhtml-syntax/parsing-xhtml-documents/xhtml-pubid-1.html - just comparing against null, but the test should really be assert_throws; the whole test looks poorly written.

html/semantics/embedded-content/media-elements/track/track-element/cors/support/common.js - test expectations are described in an object (event and array of requests); could be replaced by a dedicated comparison loop

So it's really just the IndexedDB and webmessaging code that's wanting a really generic object comparison function. We could have the function assert that the prototypes are Object.prototype or something.

But... this is really a "how do you want to evolve testharness?" question, so I need your thoughts here.

inexorabletash commented 9 years ago

Still waiting on reviews for the two PRs linked above. Anyone? @jgraham ?

sideshowbarker commented 9 years ago

@inexorabletash you wanna take care of the assert_object_equals instance in https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-workers/resources/test-helpers.js ?

inexorabletash commented 9 years ago

@sideshowbarker Hrm, I'm not seeing one.

https://github.com/w3c/web-platform-tests/search?utf8=%E2%9C%93&q=assert_object_equals

sideshowbarker commented 9 years ago

@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 service-workers/service-worker/resources/testharness-helpers.js but I see now that’s an old file laying around in my working directory, and not actually tracked by git.

So never mind about that one :-)

plehegar commented 8 years ago

We still have: ext-xhtml-pubid/the-xhtml-syntax/parsing-xhtml-documents/xhtml-pubid-1.html html/semantics/embedded-content/media-elements/track/track-element/cors/support/common.js workers/interfaces/DedicatedWorkerGlobalScope/postMessage/structured-clone-message.html

bobholt commented 7 years ago

@jgraham: Is assert_object_equals still "broken for DOM objects and undesirable in general?" If so, there's quite a bit more cleanup to do. As of 2017-03-26, assert_object_equals appears in:

/encrypted-media/Google/migrated_to_root_disabled/encrypted-media-keystatuses.html
/ext-xhtml-pubid/the-xhtml-syntax/parsing-xhtml-documents/xhtml-pubid-1.html
/html/semantics/embedded-content/media-elements/track/track-element/cors/support/common.js
/service-workers/service-worker/fetch-request-fallback.https.html
/service-workers/service-worker/navigation-redirect.https.html
/service-workers/service-worker/resources/override_assert_object_equals.js
/service-workers/service-worker/resources/testharness-helpers.js
/streams/byte-length-queuing-strategy.js
/streams/count-queuing-strategy.js
/streams/readable-streams/bad-underlying-sources.js
/streams/readable-streams/cancel.js
/streams/readable-streams/count-queuing-strategy-integration.js
/streams/readable-streams/default-reader.js
/streams/readable-streams/general.js
/streams/readable-streams/tee.js
/streams/resources/rs-test-templates.js
/webmessaging/postMessage_objects.sub.htm
/workers/interfaces/DedicatedWorkerGlobalScope/postMessage/structured-clone-message.html

And confusingly, /webmessaging also implements its own version, assert_object_equals_, in /webmessaging/support/compare.js.

At this point, would it be easier to fix what's wrong with assert_object_equals in testharness.js if it's still broken?

foolip commented 6 years ago

@jgraham, is the discussion in https://critic.hoppipolla.co.uk/showcomment?chain=12186 lost forever?

annevk commented 4 years ago

See also #20844.

It seems like assert_object_equals is broken in several ways. Should it be linted?

inexorabletash commented 4 years ago

Reviewing usage, I note the following patterns in descending order of occurrence:

shorthand for a bunch of property assertions; no need for deep comparison, and even asserting that no extra properties exist is beyond the scope of the test. e.g.

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);

the slightly stronger assertion that there are no additional own-properties, e.g.

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);

recursive array comparison, where each element is either an array or can be compared with assert_equals()
true deep-compare cases, e.g. validating postMessage()'s event.data or similar arbitrary payloads
broken usage, e.g. in workers/Worker-constructor-proto.any.js

Given the prevalence of the first two, I'd suggest adding a new simpler function to testharness.js, e.g. assert_property_values() (better name pls) which handles the most common bulk property comparison cases as a convenience. I think this accounts for 95%+ of current usage, and we can convert those over. We can then rename assert_object_equals() to something less attractive.

foolip commented 4 years ago

true deep-compare cases, e.g. validating postMessage()'s event.data or similar arbitrary payloads

Does assert_object_equals do the right thing for these? Would we need a assert_deep_equals to replace it?

inexorabletash commented 4 years ago

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 assert_deep_equals for the same reasons that assert_object_equals is a trap - it sounds convenient but will likely be either more lax or more permissive than the user is expecting, given the subtleties of JS (prototype chain, etc). So I'm in favor of having more specific functions that fail if mis-used, if we can come up with the right set.

stephenmcgruer commented 4 years ago

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!

inexorabletash commented 4 years ago

@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?

gsnedders commented 4 years ago

@jgraham, is the discussion in https://critic.hoppipolla.co.uk/showcomment?chain=12186 lost forever?

https://web.archive.org/web/20160402125934/https://critic.hoppipolla.co.uk/showcomment?chain=12186 has it, edited OP.

gsnedders commented 4 years ago

@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.

web-platform-tests / wpt

Remove all assert_object_equals instances from the suite. #2033