undefined object keys not obvious

[parisholley]

if you mock with an argument of {} and the value {foo: undefined} is used as an value, when the exception is throwing comparing the expected mock argument with actual value, it will not show {foo: undefined}, just {}, making it hard to know what the problem is right away

[parisholley]

nvm, will close till i have a reproduction

[parisholley]

  it('should support undefined values', () => {
    const expectation = new StrongExpectation('bar', [{}], { value: undefined });

    expect(expectation.matches([{key: undefined}])).toBeTruthy();
  });

this upcoming flag in typescript 4.4 explains the issue more:

https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-beta/#exact-optional-property-types

[parisholley]

this is tough because one isn't right or wrong, the current behavior of those two arguments not matching is technically correct, and while most of the time, i think a user would expect this not to be the case, for someone who does have code that depends on the existence of keys, comparing objects with undefined values removed could break things for someone else.

the printExpected method inside of jest-matcher-utils, I would expect to show key: undefined in the output, however it appears to be excluded, making it not obvious why two messages with identical diff values are showing up as not equal.

out of curiosity, I created a test to see what straight jest does, and this passes:

it('test', () => {
  expect({}).toEqual({ key: undefined });
});

so given that the jest utils AND jest expect behavior seems to line up, we'll need to either a) adjust our isEqual implementation to something that matches the jest behavior, or use a non-jest print method to mimick the lodash isEqual implementation

[parisholley]

another interesting thing, toMatchObject does fail and shows that one object has an undefined key and the other is empty:

it('test', () => {
  expect({}).toMatchObject({ key: undefined });
});

[NiGhTTraX]

@parisholley there are 2 issues described here:

1. Printing objects with undefined keys. I can't reproduce your behavior locally, explicit undefined keys are printed as expected:

const fn = mock<(x: any) => boolean>();

when(fn({})).thenReturn(true);

expect(instance(fn)({ key: undefined })).toBeTruthy();

Error: Didn't expect mock({"key": undefined}) to be called.

Remaining unmet expectations:
 - when(mock({})).thenReturn(true).between(1, 1)

2. Matching objects with missing and undefined keys. While the default comparison algorithm will not match {} and { key: undefined }, using It.isObject will:

const fn = mock<(x: any) => boolean>();

when(fn(It.isObject({}))).thenReturn(true);

expect(instance(fn)({ key: undefined })).toBeTruthy();

It.isObject is doing a partial match on the received argument so any keys not in the expected value will not cause a failure.

[parisholley]

@NiGhTTraX grrr... sorry, i still have my circular dependency patch JSON.parse(JSON.stringify()) which was wiping out the undefined keys in the message, so disregard that

w/r/t to isObject, that isn't a solution because partial matching prevents new keys showing as regressions (eg: mocking an HTTP service). i would still make the case to match jest (and other test matching behavior) by ignoring undefined by default, and use something liks It.isStrictObject (or some other name) if you care abouts keys

[NiGhTTraX]

@parisholley AFAICT only jest ignores undefined keys, while chai, sinon and Node's assert all fail comparing {} to { key: undefined } (see sandbox, switch to tests tab).

Given that jest is the outlier here, I'd keep the default behavior as is and introduce a matcher that ignores undefined keys.

[parisholley]

@NiGhTTraX that sandbox isn't comparing apples to oranges... given that strong-mock is using lodash isEqual (which is deep, not shallow), the sandbox should reflect deep matching behavior. also note the test behavior will change if you make both values the same (eg: {} vs {}))

const expected = {nested: {}};
const actual = {nested: {key: undefined}};

// shallow (fails)
assert.equal(actual, expected);

// deep (fails)
assert.deepEquall(actual, expected);

// strict equals
chai.expect(actual).to.equal(expected);

// deep (fails)
chai.expect(actual).to.deep.include(expected);

// strict
expect(spy.calledWith(expected)).toBeTruthy();

// deep (passes)
expect(spy.calledWith(sinon.match(expected))).toBeTruthy();

there is no consistency as to how any of the libs behave, some deal with undefined keys, others do not. though if we consider jest an outlier, then what is consistent, is that no library deep equals by default (strong mock does).

if we were to look at it from the angle of, excluding pure assertion libraries and only focusing on spy implementations, jest and sinon are consistent (they support undefined values when deep matching). given that strong-mock isn't intended to be an assertion/test library, i believe following their behavior would make more sense.

[NiGhTTraX]

sinon's calledWith is already deep, sinon.match has a different behavior and that's why it passes the assertion.

All object comparisons have to be deep otherwise you would just compare references. Comparing { } and { nested: { } } is the same from that perspective. I did update the sandbox though to use assert.deepEqual, which also fails.

While _.isEqual is an implementation detail of strong-mock, it is a commonly used method of deeply comparing objects so I think it's relevant to look at it.

It seems that the jest team wishes for strict equal to be the default jestjs/jest#8588 (comment), but they can't change it for risk of breaking everything jestjs/jest#6032 (comment).

The TS article you linked reinforces the need of treating an undefined key differently from a missing key, so I continue to believe that's the better default.

[parisholley]

correct, calledWidth is deep (but strict) whereas match is partial/subset

also agree that treating undefined should be default when doing object comparisons, however my disagreement is that deep comparison is the API default of strong mock.

IMO, mocking assertions should not care about the interior mutability of an object unless the users wants that behavior. my general experience across strictly-typed languages (eg: java + mockito), the default behavior of all equals() methods is effectively strict unless the user wants to override that behavior. the pitch (as i understand it) of strong mock is "strict mocking", which is a reflection of both verification behavior and explicitness, allowing "lose" matches by default is the opposite and counter-intuitive (and traversing proxies could also be problematic)/

if i pass a stateful object to a mocked method as an argument, similar to the jest team's preferences, it should be strict equal unless you state otherwise. while this project is still in its infancy, i'd say now is the time to rip off that bandaid :p

[NiGhTTraX]

@parisholley I'm not sure I understand your last comment, could you please clarify some things?

agree that treating undefined should be default when doing object comparisons

Treating how?

my disagreement is that deep comparison is the API default of strong mock

How does this relate to undefined keys?

the default behavior of all equals() methods is effectively strict unless the user wants to override that behavior

Correct, in Java/Mockito comparisons would be done by reference. However, using === in JavaScript would make most tests that use non primitives fail.

assertions should not care about the interior mutability of an object

What do you mean by interior mutability?

allowing "lose" matches by default is the opposite and counter-intuitive

What do you mean by lose? The way I see it, treating a missing key and a key with an undefined value as equal is the lose behavior here, which strong-mock avoids. strong-mock expectations are always strict in the sense that both expected arguments and received arguments have to be of the same type and have the same shape e.g. the same set of keys for objects. They do not have to have the same reference, because that would be impractical in a testing scenario.

---

It seems like we're discussing 2 separate things now — comparing missing keys vs "undefined" keys, and comparing non primitives in general. Can we please limit this issue to the undefined keys topic, and open another issue to discuss the deep comparison topic? Also, can you please clarify exactly what you expect to happen for both of them?

[parisholley]

@NiGhTTraX

Treating how?

i meant that if there was an explicit opt-in object comparison matcher, etc It.object(), treating {} and {key:undefined} as non-matching should be default. IMO, It.partial() is a more appropriate name for the current It.object() implementation, and It.object() would be an explicit opt-in to deep match.

How does this relate to undefined keys?

it relates in that, if a non-strict equals API is intended to make the library "more practical"/"easier to use", then this "javascript-friendly" approach should also assume that most cases will not care about undefined keys (json serialization, http parameters, etc).

However, using === in JavaScript would make most tests that use non primitives fail.

IMO, as they should. my point is that for a "strict" mocking library, they should fail, implicit object comparisons are always bound to have edge cases (eg: if the object is a proxy being traversed unexpectedly) and wouldn't be intuitive at first glance. this library is making a baseline assumption that "objects" are the most common arguments outside of primitives, but in a more robust application (eg: integration tests), it is the opposite. perhaps a quicker solution/thought to this point is, using something like lodash.isPlainObject() to determine when to === vs deep equal, could be a middle ground (though even trying to derive if something is an object can trigger proxies in a weird way).

What do you mean by interior mutability?

class StatefulObject {
  private state = 1;
  
  mutate(){
    this.state += 1;
  }
}

const obj = new StatefulObject();

when(mock.call(obj));

i recalled having an issue where internal state had changed during a test and caused an issue, but i'm not able to reproduce it (may have been a byproduct of some of our earlier discussions/bugs). digging through the library, it doesn't appear to "snapshot" data anywhere, so it may not show up in practice. but my point was, i should be able to call obj.mutate() as many times as i want, and it not impact the result of my tests. i would never expect a mocking library to care about the internal details of my class instances, they are not raw objects. this could go even further if you start throwing in some very obscure browser apis/instances as arguments (ArrayBuffer, WebAssembly, etc).

What do you mean by lose?

sorry typo, "loose" = non-strict equals.

treating a missing key and a key with an undefined value as equal is the lose behavior here

I agree. to recap my sentiment... non-strict equals is IMO, loose.. and therefor, comparing two objects with undefined keys, is also intuitively loose. having deep equals but strict key matching (which isn't the same as partial matching as with It.isObject()), is confusing and impractical for most use cases.

It seems like we're discussing 2 separate things now

to an extent. the potential solution to either are intertwined so i think it is easier to talk about it together. to sum up what I said above, i feel like these are some potential solutions:

Breaking API
This is my preference as there is no implicit behavior and it is clear via the API usage what is happening.

• Make strict equal the default when not using a matcher
• Rename It.object() to It.partial()
• Reintroduce It.object() using deepEquals
• Add It.trimObject() (name could be w/e) which would compare two objects with undefined values removed

Breaking Matcher
This works but it still relies on implicit behavior.

• New matcher It.strictEquals()
• Update default deepEquals matcher to clean undefined values
• Add It.strictObject() to behave like existing It.object()

Magic Path
This would fit better with the current API and likely has minimal risk to current users, but still relies on implicit behavior.

• Any non-matcher argument does a isPlainObject check, uses deepEquals or === if not a plain object
• Add library global, eg: strongmock.cleanUndefined(true) which controls deepEquals vs "cleanEquals" (my PR)

[NiGhTTraX]

using === in JavaScript would make most tests that use non primitives fail

IMO, as they should

Take the following example:

type Foo = { bar: string };
type Cb = (x: Foo) => void;

const foo = (cb: Cb) => {
  cb({ bar: "baz" });
};

const cb = mock<Cb>();
when(cb({ bar: "baz" })).thenReturn();

foo(instance(cb));

The above test will not pass using a === comparison since the object literal constructed in the expectation is different from the one constructed in the code under test. IMO this makes for a poor and unintuitive experience, and I consider the above pattern a very common one and therefore optimize for it.

this library is making a baseline assumption that "objects" are the most common arguments outside of primitives

The assumption is that for the majority of the cases where you have to deal with objects you won't expect the exact same object that you passed to your code under test e.g.:

const o = { foo: "bar" };
when(cb(o)).thenReturn();

instance(cb)(o); // pass the same object here

I consider the above pattern rare, and would introduce an It.is matcher for it.

then this "javascript-friendly" approach should also assume that most cases will not care about undefined keys (json serialization, http parameters, etc).

The examples you mentioned are indeed compelling.

---

To solve both issues discussed here, and to offer more flexibility to the user, I'll

• add a new matcher that ignores undefined keys,
• add a new matcher that uses ===,
• add a global option to specify the default matcher,
• keep the existing defaults for now to not cause a breaking change.

[parisholley]

I consider the above pattern a very common one

i would say it is ONLY common, if the argument is a primitive object. if that argument is a class instance (or any native object type provided by the javascript engine (browser or node)), i don't think anyone could make a strong case that a matcher other than === would be intuitive (see my interior mutability argument earlier in the conversation).

i would bet you could take the isPlainObject approach i mentioned above and 99% of library users would not notice a break.

The assumption is that for the majority of the cases where you have to deal with objects you won't expect the exact same object that you passed to your code under test

perhaps it would help to share the cases you speak of? while understandably, it sucks for tests to be verbose, the reward is in preventing future regressions that pass tests and type checks. while you would hope that experienced engineers are writing code and can understand when to add strict equals checks, the library is putting risk in developers knowing best practices.

here are some examples of how an inexperienced engineer can cause problems:

Service Orchestration

Original Implementation

function delegator(){
  const result1 = this.serviceA.invoke();

  const result2 = this.serviceB.invoke(result1);
}

Regression

const symbol = Symbol('foo');

function delegator(){
  const result1 = this.serviceA.invoke();

  const mutated = {...result1, [symbol]: 'bar'};

  const result2 = this.serviceB.invoke(mutated);
}

Service B may actually iterate the object in a way that recognizes symbols, but the engineer may not know that. The lodash isEqual implementation doesn't compare symbol keys and will treat this object as equal. Symbol keys are heavily utilized in state libraries and other use cases.

Transactions

Original Implementation

function delegator(tx:Transaction){
  this.serviceA.invoke(tx);
  this.serviceB.invoke(tx);
}

Regression

function delegator(tx:Transaction){
  this.serviceA.invoke(tx);
  
  const newTx = this.newTransaction();

  this.serviceB.invoke(newTx);
}

Because the Transaction type does not belong to the class under test, we cannot make any assumptions about the internal state of that object. For all we know, it is a native c library which to javascript, simply looks like {connected: true} and would always pass a deep equals test. Or in the previous example, perhaps connection is stored as a symbol, you'd have the same problem. Having fractured transactions in production would be very bad.

Derived Arguments

I think this is the case you believe to be the most common, where the delegation method is generating objects on your behalf and you want to assert those arguments. There isn't really any regression potential in this implementation. And depending on the project, you may be right, this is the most common, but there is no production risk.

function delegator(){
  const internalObject = {...};

  this.thirdPartyApi.invoke(internalObject);
}

To solve both issues discussed here

that would be great :) on my end, i would definitely take advantage of setting the default matcher to ===, or may write a simple one to do my isPlainObject ? === : _.isEqual.

either way, thanks for considering my arguments

[NiGhTTraX]

The lodash isEqual implementation doesn't compare symbol keys and will treat this object as equal

It does compare symbol keys, tests added in 2029128.

Transactions

I see your point here, though I think in practice a class like that will have some enumerable key that will be unique per instance. However, it's entirely conceivable that it wouldn't, leaving it up to the user to opt in to a stricter matcher. With the latest release this should be easy.

---

@parisholley please check out release 7.3.0 and let me know if it's flexible enough for you. I cherry picked your changes from #257 and released them under a new { strict: false } flag for the It.deepEquals matcher. You can change its default options, or use a different default matcher with the new setDefaults method.

[parisholley]

It does compare symbol keys

bah your right, i missed his comment of the patch here: lodash/lodash#2840

@NiGhTTraX implementation wise, looks good :)

[parisholley]

got it do what I need :) fyi, this is the custom matcher I am using as a hybrid approach i mentioned before

const isClassInstance = (arg) =>
  typeof arg === 'object' &&
  !Array.isArray(arg) &&
  arg !== null &&
  arg.constructor.name !== 'Object' &&
  typeof arg !== 'string' &&
  typeof arg !== 'number';

const matcher = (expected) =>
  strong.It.matches(
    (arg) => {
      if (isClassInstance(expected)) {
        if (expected.constructor.name === 'Big') {
          if (!arg || arg.constructor.name !== 'Big') {
            return false;
          }

          return expected.eq(arg);
        }

        return strong.It.is(expected).matches();
      }

      return strong.It.deepEquals(expected).matches(arg);
    },
    {
      toJSON: () => {
        if (isClassInstance(expected)) {
          return strong.It.is(expected).toJSON();
        }

        return strong.It.deepEquals(expected).toJSON();
      },
    }
  );

strong.setDefaults({ matcher });