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

Add assertion that just compares specific items in the tree #845

Closed
jamestalmage opened this issue May 20, 2016 · 42 comments · Fixed by #2490
Closed

Add assertion that just compares specific items in the tree #845

jamestalmage opened this issue May 20, 2016 · 42 comments · Fixed by #2490
Labels
enhancement new functionality 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted scope:assertions

Comments

@jamestalmage
Copy link
Contributor

jamestalmage commented May 20, 2016

Issuehunt badges

This would be especially handy for AST type tests.

Consider the following

const ast = {
  loc: ... // I usually don't care about this.
  start: ... // or this
  end: ... // or this
  type: 'MemberExpression', // usually I DO care about this
  object: {
     // maybe I care about the contents of this, maybe I don't
  },
  property: {
    // again, maybe
  }
};

It would be nice to have a sort of "deepStrictEqual but only for the portion of the graph I specify"

I am going to use t.like:

t.like(ast, {
  type: 'MemberExpression',
  object: {
    type: 'Identifier',
    name: 'foo'
  }
});

In the above, I'm basically saying "I want to verify it's a MemberExpression Node, whose object property is an Identifier with name foo". loc, start, end are ignored throughout, as is the property property.


IssueHunt Summary

futpib futpib has been rewarded.

Backers (Total: $80.00)

Submitted pull Requests


Tips

@novemberborn
Copy link
Member

A bit like Sinon.JS matchers?

@jfmengels
Copy link
Contributor

For reference, looks a lot like Lodash' _.isMatch and _.matches (same thing, but the former does the check, while the latter returns a function that does the check)

@nfcampos
Copy link
Contributor

I'd find this pretty useful 👍

@jamestalmage
Copy link
Contributor Author

Yes. I was thinking of Sinon when coming up with this.

@jfmengels
Copy link
Contributor

For naming:

  • Lodash, Expect & Sinon use the term match (matches, isMatch...)
  • Ramda, Expect & Chai use match for matching in a string
  • Should.js uses contain
  • I have seen the term shape used for something similar (for proptypes, @kentcdodds's lib)

(Expect uses match for both strings and objects)

@kentcdodds
Copy link
Contributor

So far, I've just used isMatch from lodash with AVA. Would be pretty handy to have built-in

@jamestalmage
Copy link
Contributor Author

jamestalmage commented May 20, 2016

I do like that Sinon allows you to attach predicates. What if we allowed the same thing, but they exactly matched existing assertions:

t.match(obj, {
  foo: t.match.is('foo') // obj.foo must be strict equal
  bar: t.match.deepEqual({baz: 'quz'}), // traditional deepEquals, must only have the baz property
  baz: t.match.throws(), // a rejected promise?
  quz: t.match.regex(),
  promise: t.match.then.deepEqual() // same as t.match, but for promise values?
});

Essentially, t.match.XXX becomes a way to curry assertions with the expected argument.

We could of course provide shortcuts. Primitives would shortcut to t.match.is, and object literals would shortcut to t.match.match

t.match(obj, {
  foo: 'bar',  // equivalent to t.match.is('bar')
  bar: {   // not the same as above. Equivalent to `t.match` instead of `deepEquals`.  
    baz: 'quz' // We only check the baz, property, ignoring the rest
  }
});

@jamestalmage
Copy link
Contributor Author

It would be awesome to allow you to build reusable predicates this way, so I think you expose the predicate builder on test as well.

const isIdentifier = name => test.match({
  type: 'Identifier',
  name
});

const isT = isIdentifier('t');

const isAssertion = name => test.match({
  type: 'CallExpression',
  callee: {
    type: 'MemberExpression',
    object: isT,
    property: isIdentifier(name)
  }
});

const isThrowsAssertion = isAssertion('throws');

test(t => {
  // ...
  t.match(node, isThrowsAssertion)
});

@jfmengels
Copy link
Contributor

jfmengels commented May 20, 2016

The last proposal is not more reusable that just creating a new function.

const isIdentifier = name => ({
  type: 'Identifier',
  name
});

const isT = isIdentifier('t');

const isAssertion = name => ({
  type: 'CallExpression',
  callee: {
    type: 'MemberExpression',
    object: isT,
    property: isIdentifier(name)
  }
});

const isThrowsAssertion = isAssertion('throws');

test(t => {
  // ...
  t.match(node, isThrowsAssertion)
});

(exact same thing, just removed test.match)
Looks simpler to me, and it doesn't introduce new APIs (not counting t.match obviously)

@jamestalmage
Copy link
Contributor Author

jamestalmage commented May 20, 2016

Your right.

@vadimdemedes
Copy link
Contributor

I like the idea of t.like() to do partial comparisons, but t.match sounds complicated to me and would need "good" explanation to users.

@jamestalmage
Copy link
Contributor Author

I don't think we would add two separate function names. The question is whether leaf nodes of the expected argument must be primitives checked with ===, or if we allow something a bit more flexible.

@vadimdemedes
Copy link
Contributor

I did not mean the name t.match sounds complicated, but rather proposed functionality.

I liked the idea behind t.like to not compare objects 1:1, but only a subset of first-level keys. See the example in the first post.

@novemberborn
Copy link
Member

To be fair I've always found sinon.match rather confusing. t.like() comparing a subset of first-level keys is easy to understand, and could be a nice initial guard. Then you could add more assertions using destructuring.

@kentcdodds
Copy link
Contributor

Yeah, I vote for implementing the API described in the initial posting. t.match seems too complicated to fit the simplicity of the rest of ava's APIs

@jfmengels
Copy link
Contributor

I agree with with implementing the proposed t.like, though I'd call it t.match to be consistent with the other libraries I've seen.

@novemberborn
Copy link
Member

Chai uses match for regular expressions.

@jfmengels
Copy link
Contributor

See #845 (comment). A lot of them use match for this, some use match for strings (and some for both).

@kentcdodds
Copy link
Contributor

I've always used: https://www.npmjs.com/package/chai-subset

containsSubset seems OK but long. I'd be fine with like. Either way is good with me.

@jamestalmage
Copy link
Contributor Author

jamestalmage commented May 22, 2016

Let's do like. I think it implies a weaker comparison than match does (and it is indeed a weaker comparison than deepEqual)

@vadimdemedes vadimdemedes added the enhancement new functionality label May 28, 2016
@leewaygroups
Copy link

Let me get my hands dirty on this.

@novemberborn
Copy link
Member

@leewaygroups cool!

I'm rereading the thread now and I can't figure out where we landed with deep objects. Perhaps we'll follow https://lodash.com/docs/4.16.6#isMatch as suggested by @jfmengels in #845 (comment)? We already use https://lodash.com/docs/4.16.6#isEqual in t.deepEqual. What was your take @leewaygroups?

@leewaygroups
Copy link

First, I agree with the idea of maintaining a simple interface so I think the name like is fine.

Since lodash is one of the dependencies used, @jfmengels suggestion _.isMatch seem very appropriate. However, @kentcdodds auggestion containsSubset is what I consider more robust.

My take is, adopting a simple name like and building this based on chai-subsets containsSubset.
Drawback: 'chai-subset' is dependent on chai.

What do you think?

@mightyiam
Copy link
Contributor

Please be efficient with dependencies.
Lodash is seems responsive and kind. Perhaps your changes will be welcomed there.

@novemberborn
Copy link
Member

Drawback: 'chai-subset' is dependent on chai.

That's a show-stopper, unfortunately. Why do you consider it to be more robust than lodash.ismatch?

Since lodash is one of the dependencies used

We don't depend on all of Lodash, so regardless we'd be adding a new dependency. That said, using modules of the same high-quality origin for our assertions seems like a good idea.

@mightyiam
Copy link
Contributor

Are we green 🚥 for lodash.ismatch? I've just used it inside t.true in a project.

@mightyiam
Copy link
Contributor

t.deepIncludes?

@leewaygroups
Copy link

leewaygroups commented Jan 18, 2017

The pros for Lodash is compelling and green. After previous converstaions went with lodash. I'll checkin the changeset for review soon.

@mightyiam @novemberborn Thanks for the follow up.

@frantic1048
Copy link

frantic1048 commented Apr 21, 2017

@mightyiam How do you get the full object printed in terminal with t.true(isMatch(report, match)) ?

I'm testing tree like structure with same method, getting output like this:

   51:   }                                                              
   52:   t.true(isMatch(actual, expected), 'should parse two paragraph')
   53: })                                                               

  should parse two paragraph

  Value is not `true`:

    false

  isMatch(actual,expected)
  => false

  expected
  => Object {
    ast: [Array],
  }

  actual
  => Object {
    ast: [Array],
    error: null,
  }

That's painful that I cannot recognize what is failing isMatch(). Versions are:

  • ava@0.19.1
  • lodash.ismatch@4.4.0

@novemberborn
Copy link
Member

As of #1341 we're no longer using lodash.isequal. This feature will require partial comparison support in https://github.com/concordancejs/concordance.

@frantic1048
Copy link

Finally I wrapped an isMatch() for logging ...

const im = require('lodash.ismatch')
const chalk = require('chalk')

const inspect = require('util').inspect

function isMatch (actual, expected) {
  const res = im(actual, expected)
  if (res === false) {
    console.log(chalk.green('\nexpected:'))
    console.log(inspect(expected, { depth: null }))
    console.log(chalk.red('\nactual:'))
    console.log(inspect(actual, { depth: null }))
    console.log()
  }
  return res
}

@norbertkeri
Copy link

Did this get resolved? Is there a way to do partial matches? The suggestion by @frantic1048 and @mightyiam both produce erroneous output in the console when used with t.true().

@novemberborn
Copy link
Member

@norbertkeri nope. It requires a solid proposal as to how this would work, ensuring the same kind of diff output as deepEqual gives us.

With regards to @frantic1048's example, if you can pass it the t object you can use t.log() which should work better.

@norbertkeri
Copy link

I can use t.log() for formatting and displaying the error, but I would still need to put an assertion somewhere, and using t.true() is causing the erroneous output. Is there something I'm missing with t.log()?

@novemberborn
Copy link
Member

using t.true() is causing the erroneous output

By erroneous you mean "not enough detail"? That's a hard one to solve, hence the approach advocated by others.

The real solution is to have a partial-match assertion, of course.

@norbertkeri
Copy link

No, using this code:

    t.true(isMatch(result, { currentUser: { email: 'hello@world.com' }}));

Produces this output:

  1 failed [10:47:10]

  currentUser › Current user is returned

  /home/myuser/projects/x/usermanagement/tests/currentUser.test.js:38

   37:         .queryData("{ currentUser { id, email }}");                       
   38:     t.true(isMatch(result, { currentUser: { email: 'hello@world.com' }}));
   39: });                                                                       

  Value is not `true`:

  false

  isMatch(result, {
    currentUser: {
      email: 'hello@world.com'
    }
  })
  => false

  {
    currentUser: {
      email: 'hello@world.com'
    }
  }
  => {
    currentUser: {
      email: 'hello@world.com',
    },
  }

  {
    email: 'hello@world.com'
  }
  => {
    email: 'hello@world.com',
  }

  result
  => {
    currentUser: {
      email: 'hello@bye.com',
      id: '1',
    },
  }

Which all comes from the t.true() call (from what I assume because of the magic assert feature).

@frantic1048
Copy link

@norbertkeri Which version of isMatch are you using? from lodash or the example in #845 (comment)

Here's my usage of doing partial matching:

https://github.com/frantic1048/Est/blob/6f0b24584f54809c197f00f4bd2282eee1c9744f/test/grammar.BulletList.js#L54

I just wrapped lodash's isMatch a little to generate detailed log. It is not very ergonomic but prints enough data for debugging.

@norbertkeri
Copy link

@frantic1048 the example from the comment. Could you show me what output do you get when the test fails (actual doesn't match expected)?

@frantic1048
Copy link

@norbertkeri For example, I deleted one line of input text to parse(), which makes actual lackes one ListItem at https://github.com/frantic1048/Est/blob/6f0b24584f54809c197f00f4bd2282eee1c9744f/test/grammar.BulletList.js#L28

t.true() generates error info like this(log of tracer.log() is omitted because it's quite long and not related to this issue):

(expected needs to match 3 ListItem where actual has 2 ListItem)

expected:
{ ast:
   { T: Symbol(Document),
     C:
      [ { T: Symbol(BulletList),
          C:
           [ { T: Symbol(ListItem) },
             { T: Symbol(ListItem) },
             { T: Symbol(ListItem) } ] } ] } }

actual:
{ ast:
   { ctx: ASTYCtx { ASTYNode: [Function] },
     ASTy: true,
     T: Symbol(Document),
     L: { L: 1, C: 1, O: 0 },
     A: {},
     C:
      [ { ctx: ASTYCtx { ASTYNode: [Function] },
          ASTy: true,
          T: Symbol(BulletList),
          L: { L: 1, C: 1, O: 0 },
          A: {},
          C:
           [ { ctx: ASTYCtx { ASTYNode: [Function] },
               ASTy: true,
               T: Symbol(ListItem),
               L: { L: 1, C: 3, O: 2 },
               A: {},
               C:
                [ { ctx: ASTYCtx { ASTYNode: [Function] },
                    ASTy: true,
                    T: Symbol(Paragraph),
                    L: { L: 1, C: 3, O: 2 },
                    A: {},
                    C:
                     [ { ctx: ASTYCtx { ASTYNode: [Function] },
                         ASTy: true,
                         T: Symbol(Text),
                         L: { L: 1, C: 3, O: 2 },
                         A: { value: 'item1' },
                         C: [],
                         P: [Circular] } ],
                    P: [Circular] } ],
               P: [Circular] },
             { ctx: ASTYCtx { ASTYNode: [Function] },
               ASTy: true,
               T: Symbol(ListItem),
               L: { L: 3, C: 3, O: 11 },
               A: {},
               C:
                [ { ctx: ASTYCtx { ASTYNode: [Function] },
                    ASTy: true,
                    T: Symbol(Paragraph),
                    L: { L: 3, C: 3, O: 11 },
                    A: {},
                    C:
                     [ { ctx: ASTYCtx { ASTYNode: [Function] },
                         ASTy: true,
                         T: Symbol(Text),
                         L: { L: 3, C: 3, O: 11 },
                         A: { value: 'item2' },
                         C: [],
                         P: [Circular] } ],
                    P: [Circular] } ],
               P: [Circular] } ],
          P: [Circular] } ],
     P: null },
  error: null }

@norbertkeri
Copy link

Ok I'm not sure what's going in my code then, the output I get is very different (noted above). Will take a look later, thanks.

@sindresorhus sindresorhus changed the title An assertion that just compares specific items in the tree. Add assertion that just compares specific items in the tree May 2, 2019
@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 10, 2019
@IssueHuntBot
Copy link

@IssueHunt has funded $80.00 to this issue.


@issuehunt-oss
Copy link

issuehunt-oss bot commented Jun 14, 2020

@novemberborn has rewarded $72.00 to @futpib. See it on IssueHunt

  • 💰 Total deposit: $80.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $8.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new functionality 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted scope:assertions
Projects
None yet