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

allow using matchers inside contains #317

Merged
merged 1 commit into from
Dec 26, 2017

Conversation

lgandecki
Copy link
Contributor

This allows us to do things like:

brew({nested: {nestedString: 'abc', irrelevant: true}, irrelevantHere: 'alsoTrue'})

td.verify(brew(td.matchers.contains({irrelevantHere: td.matchers.isA(String)})))
td.verify(brew(td.matchers.contains(
{nested: td.matchers.contains({nestedString: td.matchers.isA(String)})}
)));

Before, the tests I added were failing (basically, you weren't able to mix matcher.contains with other matchers (including another matcher.contains as showed above)

@searls
Copy link
Member

searls commented Dec 25, 2017

Very cool! I think @Schoonology requested this a while back

@searls
Copy link
Member

searls commented Dec 25, 2017

I just added a couple additional checks to this branch, and found one issue: that this does not apply to arrays as one would expect. Here's the failing test I added:

this.matches(td.matchers.contains(td.matchers.isA(Number)), ['a', 3, 'b']) === true

https://github.com/testdouble/testdouble.js/blob/lgandecki-containsMatchers/regression/src/matchers-test.coffee#L91

I'll take a look to see if this is a trivial addition to the function you changed

@searls
Copy link
Member

searls commented Dec 25, 2017

I didn't have much luck making this behave consistently for arrays (the complexity of the private function is now at a point where it seems a single recursive function would be preferable to two half-functions given that _.isEqual checks will no longer cut it, but I didn't have time this morning to work out a good solution. I did refactor it slightly in 7e7b243 before quitting, though

@lgandecki
Copy link
Contributor Author

Thanks for the feedback!
I don't really need the array option for the thing I'm working on currently, but I understand the need for consistency here. :)

I was able to make the tests pass with a bit extra complexity.. We could possibly split those in separate sub functions. I tried to break this but couldn't. My examples:

const brewNew = td.function();
  brewNew(['sdf', {test: 'abc'}, 4, {testNew: 'sdf'}, {testAlso: {stringy: 'string', number: 4}}]);

  td.verify(
    brewNew(
      td.matchers.contains(
        {test: 'abc'},
        td.matchers.isA(String),
        td.matchers.isA(Number),
        {testNew: td.matchers.isA(String)},
        {
          testAlso: td.matchers.contains({
            stringy: 'string', number: td.matchers.isA(Number)
          }
          )
        }
      )
    )
  );
  const newer = td.function();

  newer({
    lol: {
      deep:
          [{ad: ['wer', {evenDeeper: [function () {}, 4]}, 3]}, 2]
    },
    other: 'stuff'
  });

  td.verify(newer(td.matchers.contains({
    lol: {
      deep: [{
        ad: ['wer', {evenDeeper: td.matchers.contains(td.matchers.isA(Number), 4)}]
      }]
    }
  })));

@lgandecki
Copy link
Contributor Author

I just realized that if I simplify the code more, to:

var containsAllSpecified = (containing, actual) => {
  if (_.isArray(actual)) {
    return _.some(actual, el => {
      if (isMatcher(containing)) {
        return containing.__matches(el)
      }
      return containsAllSpecified(containing, el)
    })
  }
  return actual != null && _.every(containing, (val, key) => {
    if (isMatcher(val)) {
      return val.__matches(actual[key])
    }
    if (_.isArray(containing)) {
      return containsAllSpecified(val, actual)
    }
    if (_.isObjectLike(val)) {
      return containsAllSpecified(val, actual[key])
    } else {
      return _.isEqual(val, actual[key])
    }
  })
}

(removing the && !_.isArray(containing) check in the second line and wrapping the containing as an array)
I get the built-in tests to pass, but my safety net failed:

const newer = td.function();

  newer({
    lol: {
      deep:
          [{ad: ['wer', {evenDeeper: [function () {}, 4]}, 3]}, 2]
    },
    other: 'stuff'
  });

  td.verify(newer(td.matchers.contains({
    lol: {
      deep: [{
        ad: ['wer', {evenDeeper: td.matchers.contains(td.matchers.isA(Number), 4)}]
      }]
    }
  })));

So I've added a test to cover a more complex example like this one. I switched from a function inside the array to an object - I couldn't figure out how to make something like that in coffeescript, and didn't particularly fancy learning that ;-)

@searls searls merged commit de57c8d into testdouble:master Dec 26, 2017
@lgandecki
Copy link
Contributor Author

Hey, thanks for merging @searls ! The recent changes I talked about were here:
https://github.com/testdouble/testdouble.js/pull/318/files

Not sure if you decided it was making thet initially function too complex, or assumed my changes were here :-)

@searls
Copy link
Member

searls commented Dec 26, 2017

I iterated on this a bit more today and was able to simplify things quite a bit further. Just cut a release. Thanks for this contribution!!

Landed in 3.3.0.

@lgandecki
Copy link
Contributor Author

Right, I see now. Much cleaner - great work :-) Thanks for this fantastic library - it, and it's docs, changed the way I think about testing.

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

Successfully merging this pull request may close these issues.

None yet

2 participants