STREAMS-1982: Add singleton compat for MinKey #23
Conversation
|
@addaleax 👋 could I get a review when you get a chance? I don't have the ability to add reviewers |
tcannon91
force-pushed
the
STREAMS-1982
branch
from
0942c69 to
c1a582c
Compare
| // Test that multiple references return the same instance | ||
| const anotherMinKeyRef = new MinKey(); | ||
| assert.strictEqual(minKey, anotherMinKeyRef, "minKey should be a singleton - v1"); | ||
| assert.strictEqual(MinKey(), MinKey(), "minKey should be a singleton - v2"); |
There was a problem hiding this comment.
You'll need to make sure that MinKey still serializes properly to BSON/EJSON without calling it
There was a problem hiding this comment.
Can you clarify what you mean by "without calling it"? If you have a reference test I can look at, that would be helpful
There was a problem hiding this comment.
I have something like this, but I am not certain if it accomplishes what you are looking for 😓
const serializedBsonMinKey = bson.serialize({ key: minKey });
const deserializedBsonMinKey = bson.deserialize(serializedBsonMinKey);
assert.deepEqual(minKey, deserializedBsonMinKey.key, "should be equal after bson serialization");
const serializedEJSONMinKey = EJSON.stringify({ key: minKey });
const deserializedEJSONMinKey = EJSON.parse(serializedEJSONMinKey);
assert.deepEqual(minKey, deserializedEJSONMinKey.key, "should be equal after ejson serialization");
There was a problem hiding this comment.
Can you clarify what you mean by "without calling it"?
One oddity of MongoDB JS environments like the legacy shell and $function is that
db.coll.insertOne({ a: MinKey, b: MaxKey })and
db.coll.insertOne({ a: MinKey(), b: MaxKey() })are equivalent – and it's not immediately true whether that's still the case after this patch or not; and even if it's the case already, I'd say we should still add a test to make sure it keeps being true regardless of future modifications to this code
I have something like this, but I am not certain if it accomplishes what you are looking for
Right, doing that with those values should be fine 🙂
(Aside from the fact that you almost certainly want assert.deepStrictEqual instead of assert.deepEqual)
There was a problem hiding this comment.
Let me know what you think of the changes. I adjusted the test from what I originally included in the thread.
- I did a different comparison that I think better captures the nuance of the example you shared
- I removed the ejson stringify/parse because I couldn't get it working (😖) but also I don't think it was important related to your shared example. The bson parsing seemed to be what mattered.
There was a problem hiding this comment.
EJSON would be kind of nice to test since it's built into mongosh (unlike direct bson serialization/deserialization) but that's fine, yeah
| assert(minKey instanceof MinKey, "minKey should be an instance of MinKey"); | ||
| assert.strictEqual(minKey.tojson(), '{ "$minKey" : 1 }', "minKey should serialize correctly"); | ||
| assert.strictEqual(minKey.toString(), "[object Function]", "minKey toString should work"); | ||
| assert.strictEqual(minKey.toJSON(), '{ "$minKey" : 1 }', "minKey toJSON should work"); |
There was a problem hiding this comment.
Not a big deal, but passing a static string as a message for assert.strictEqual() and friends is an antipattern since it will hide information about the actual comparison results – if you want to add this type of information, add a comment
| // Test that multiple references return the same instance | ||
| const anotherMinKeyRef = new MinKey(); | ||
| assert.strictEqual(minKey, anotherMinKeyRef, "minKey should be a singleton - v1"); | ||
| assert.strictEqual(MinKey(), MinKey(), "minKey should be a singleton - v2"); |
There was a problem hiding this comment.
EJSON would be kind of nice to test since it's built into mongosh (unlike direct bson serialization/deserialization) but that's fine, yeah
2 participants
No description provided.