avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

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

Closed jamestalmage closed 4 years ago

jamestalmage commented 8 years ago

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](https://issuehunt.io/u/futpib) has been rewarded. ### Backers (Total: $80.00) - [issuehunt issuehunt](https://issuehunt.io/u/issuehunt) ($80.00) ### Submitted pull Requests - [#2490 Add `like` assertion (fix #845)](https://issuehunt.io/r/avajs/ava/pull/2490) --- ### Tips - Checkout the [Issuehunt explorer](https://issuehunt.io/r/avajs/ava/) to discover more funded issues. - Need some help from other developers? [Add your repositories](https://issuehunt.io/r/new) on IssueHunt to raise funds.
novemberborn commented 8 years ago

A bit like Sinon.JS matchers?

jfmengels commented 8 years ago

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 commented 8 years ago

I'd find this pretty useful 👍

jamestalmage commented 8 years ago

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

jfmengels commented 8 years ago

For naming:

(Expect uses match for both strings and objects)

kentcdodds commented 8 years ago

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

jamestalmage commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Your right.

vadimdemedes commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Chai uses match for regular expressions.

jfmengels commented 8 years ago

See https://github.com/avajs/ava/issues/845#issuecomment-220611096. A lot of them use match for this, some use match for strings (and some for both).

kentcdodds commented 8 years ago

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 commented 8 years ago

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

leewaygroups commented 7 years ago

Let me get my hands dirty on this.

novemberborn commented 7 years ago

@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 https://github.com/avajs/ava/issues/845#issuecomment-220549923? We already use https://lodash.com/docs/4.16.6#isEqual in t.deepEqual. What was your take @leewaygroups?

leewaygroups commented 7 years ago

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 commented 7 years ago

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

novemberborn commented 7 years ago

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 commented 7 years ago

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

mightyiam commented 7 years ago

t.deepIncludes?

leewaygroups commented 7 years ago

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 commented 7 years ago

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

novemberborn commented 7 years ago

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

frantic1048 commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@norbertkeri Which version of isMatch are you using? from lodash or the example in https://github.com/avajs/ava/issues/845#issuecomment-312860196

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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.

IssueHuntBot commented 5 years ago

@issuehunt has funded $80.00 to this issue.


issuehunt-oss[bot] commented 4 years ago

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