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

jest matchers arent exposed to be extended #2547

Closed
yoavniran opened this issue Jan 10, 2017 · 32 comments
Closed

jest matchers arent exposed to be extended #2547

yoavniran opened this issue Jan 10, 2017 · 32 comments

Comments

@yoavniran
Copy link
Contributor

yoavniran commented Jan 10, 2017

Do you want to request a feature or report a bug?
Feature

What is the current behavior?

Im looking into adding my own custom matchers.
for example, to me an obvious missing one is a shortcut for toHaveBeenCalledTimes(1) as toHaveBeenCalledOnce() or just toBeCalledOnce().

I set out to implement it, thinking it would be just a matter of internally calling toHaveBeenCalledTimes(1) and thats it.

However, turns out, the raw matchers from jest-matchers arent actually exposed to the outside world.

I can see they are available on global[GLOBAL_STATE].matchers but no method available to expose them in the same way getState() does for global[GLOBAL_STATE].state.

is this intentional or an oversight?

What is the expected behavior?

Would be nice to have the raw matchers available through the expect object just like getState and the new asymmetric matchers

More accurately, make the wrapped matchers by 'makeThrowingMatcher' available to the outside.

(running on Jest 18.1.0)

@thymikee
Copy link
Collaborator

Use expect.extend

@yoavniran
Copy link
Contributor Author

hi @thymikee i think im either missing something or wasnt clear before.

expect.extend is what I used. But all it does is assign new matchers to the internal matchers object.
It doesnt provide a way to call an existing matcher from my custom one.

Am i wrong?

@cpojer
Copy link
Member

cpojer commented Jan 10, 2017

I don't think we want to expose the original matchers; they are not supposed to compose. However, we expose a bunch of methods on this that you can help to create your own matchers. What functionality are you looking for exactly?

@yoavniran
Copy link
Contributor Author

well, i havent gone much beyond the one use-case i mentioned above: being able to shortcut toHaveBeenCalledTimes(1)
However, in general I find it useful to have these things extendable/composable as it gives rise to useful and sometimes unexpected use-cases.

Whats the reasoning for not exposing the original matchers? I cant see the harm in having them available if someone wants to wrap or compose...

@cpojer
Copy link
Member

cpojer commented Jan 10, 2017

It basically means we have to maintain a certain API for them when I consider them an implementation detail. I think I'd prefer to compose them like this:

myMatcher = (received, expected) => {
  expect(received).toBe(expected);
  
  // Do my own custom stuff.
};

if you want to customize the output, you can try-catch the expect call.

@yoavniran
Copy link
Contributor Author

I thought about that but its not as elegant as id hoped.
the missing part for me is that I cant simply do return ... within my custom matcher.

something like:

myMatcher = (received, expected) => expect.matchers.existingMatcher(received, (expected+1));

Actually, I'd argue they are already part of the public API - they are exposed (not as raw) on the return value of expect(). Therefore, I dont see it as a big step to also make them available as raw matchers.

@cpojer
Copy link
Member

cpojer commented Jan 10, 2017

@DmitriiAbramov do you have any thoughts?

@aaronabramov
Copy link
Contributor

@yoavniran if you return a raw matcher result, wouldn't you end up with wrong error messages?
like.. in your example if you call it as myMatcher(1, 2)
the message will say received: 1, expected: 3

@yoavniran
Copy link
Contributor Author

yoavniran commented Jan 10, 2017

@DmitriiAbramov the last example was just a silly one, I started this thread with the idea of having a custom shortcut for toHaveBeenCalledTimes, like toBeCalledOnce.
If I had the raw matcher Id have the control so for my example: toBeCalledOnce, toBeCalledTwice, etc. the message from the existing matcher would be perfect.
For other cases, Id be able to inspect the result and decide what should happen.

@aaronabramov
Copy link
Contributor

should we just add those shortcuts to the core matchers? :)
i really don't see many other use cases for that. most of the time it's easier to just create a new matcher and share code internally, rather than through some API that exposes raw matchers

@yoavniran
Copy link
Contributor Author

Im in favor of adding them to the core matchers. for instance up to 4 or 5 times which should cover most of the checks made.

However, unless im mistaken, the API for exposing the internal matchers amounts to:

//packages/jest-matchers/src/index.js
expect.getMatchers = () => ({...global[GLOBAL_STATE].matchers});

so I really dont see the harm or overhead in exposing them for the community

@yoavniran
Copy link
Contributor Author

hi @DmitriiAbramov , @cpojer
just so this isnt forgotten, can let me know what you think should be done and reopen if needed?

thanks.

@cpojer
Copy link
Member

cpojer commented Jan 15, 2017

I still agree with @DmitriiAbramov that I don't see the value. Can you explore exposing the functionality you need through other ways?

@yoavniran
Copy link
Contributor Author

hi @cpojer
I think @DmitriiAbramov saw the value in the called-times shortcuts i mentioned. Ill be happy to create a PR for those to be in the core matchers.

As for exposing the core matchers for extensibility purposes, I dont really understand your reluctance as i pointed out its extremely simple to expose and i dont see support overhead but with the shortcuts in place Ill be happy (for now)

@aaronabramov
Copy link
Contributor

exposing just "one more simple thing" always creates a lot of edge cases that need to be accounted for. That usually creates a pretty big overhead when adding new features or refactoring, so i'd rather not expose anything extra unless we really need to.
@cpojer what do you think about creating calledOnce types of matchers?

@gaearon
Copy link
Contributor

gaearon commented Nov 22, 2017

I wanted to write a custom matcher for amending toThrow behavior. I looked at existing toThrow matcher and didn't want to fork it (thus losing any bugfixes, supported argument overloads, etc). So I guess I won't do it. (Just tossing this out as an example for this being helpful.)

@bradleyayers
Copy link

I wanted to write a custom matcher for comparing ProseMirror EditorState objects. These objects have a handful of different properties that require a few different styles of comparison. For some I use ProseMirror's builtin .eq() comparisons, but for others I'd like to just use toMatchObject. I think having a way to compose existing matchers would be very valuable in this scenario.

For now I'll cobble together (copy/paste) snippets from the existing matchers to get the job done.

@SimenB
Copy link
Member

SimenB commented Dec 1, 2017

The toThrow matchers are pretty special, but most other matchers is just a comparison and using stuff from jest-matcher-utils to make pretty error messages. What reuse do people want?

If you really want to reuse other matchers, I don't understand what's wrong with the suggestion in #2547 (comment)

const myFancyErrorMatcher = (received, expected, somethingElse) => {
  try {
    expect(received).toThrow(expected);
    expect(received.someProperty).toBe(true);
  } catch (e) {
    // something failed, check the error and change stuff in it if needed and rethrow it
  }
};

I'd love to see specific examples of what folks want to achieve, and why it's difficult now.

@Undistraction
Copy link

I'd love to see specific examples of what folks want to achieve, and why it's difficult now.

FWIW I have a simple use-case. I want to make a change to the expected value before submitting it to toEqual so that I can write my expects using multiline template strings and know the excess whitespace will be trimmed. Here is what I hoped would work:

import indentToFirstLine from 'indent-to-first-line'
import expect from 'expect'

const toEqualMultiline = (received, expectedTemplateString) => {
  const expected = indentToFirstLine(expectedTemplateString)
  return expect(received).toEqual(expected)
}

export default toEqualMultiline

Obviously this doesn't work because expect() doesn't return anything.

I realise it isn't a big deal to copy/paste from the toEqual matcher and create my own from scratch, but it would be nice to be able to do the above.

@rickhanlonii
Copy link
Member

So, I implemented this as a proof of concept (1 line change)

it("basic test", () => {
  const actual = { deep: { prop: { path: 'foo' } } };
  expect(actual).toBeEqualHelper('foo');
});

expect.extend({
  toBeEqualHelper(received, val, matchers) { 
    return matchers.toEqual.call(this, received.deep.prop.path, val);
  },
});

It works great in the success case, but as @aaronabramov pointed out, it gets weird in the failure case:

Notice the top line

expect(received).toBe(expected) // Object.is equality

That's a big wat, and only gets weirder with more complex uses 😞


I agree that there's a real pain point here. For example, this PR for a toMatchSnapshotAndLog matcher was trivial to implement inside of Jest, but would require cloning the toMatchSnapshot to implement as a custom matcher

Jasmine 2 solves this with custom matcher factories that are passed util functions

For us that may look something like:

expect.extend({
  toBeEqualHelper(util) { 
    return function(received, val) {
      const result = util.equals(received.deep.prop.path, val);

      if (pass) {
        return {...result, message: 'Helper message pass' }
      } else {
        return {...result, message: 'Helper message fail' }
      }
    }
  },
  toMatchSnapshotAndLog(util) { 
    return function(received, val) {
      const result = util.snapshot(received);
      console.log(result.snapshot);

      return {...result };
    }
  },
});

@SimenB
Copy link
Member

SimenB commented Mar 18, 2018

this.utils is already available inside custom matchers. We could possibly add some snapshot stuff to it as well?

@rickhanlonii
Copy link
Member

Ah, good point @SimenB

There are some spy matcher utils we could expose too

@rightqa
Copy link

rightqa commented Mar 27, 2018

@SimenB @rickhanlonii are there any plans to do this? I mean adding snapshot stuff to this.utils?

This is what I am trying to do.

Make an API call
Get the response
Scrub the response (remove all data, just keep the structure of response)
and then using scrubbed response toMatchSnapshot.

This is working fine, but one of the team members suggested if we can extend the expect something like
expect(response).toMatchScrubbedSnapshot();

and in the toMatchScrubbedSnapshot, I would actually scrub the values and use this.utils.toMatchSnapshot and then return the results.

Is there an another way to achieve, what I am trying to do? or I will have to wait this this.utils include snapshot stuff?

I am happy to raise a PR to include toMatchScrubbedSnapshot() in the core itself, if you guys feel it's useful to wider community?

Your response will be highly appreciated.

@SimenB
Copy link
Member

SimenB commented Mar 28, 2018

Vague plans, but nobody's working on it. I'm not sure which parts are needed, or useful, or if it's even possible.

The scrub thing sounds like #5847.

@haffmaestro
Copy link

As another an example of where the current solutions falls short.

Using @SimenB's example;

const myFancyErrorMatcher = (received, expected, somethingElse) => {
  try {
    expect(received).toThrow(expected);
    expect(received.someProperty).toBe(true);
  } catch (e) {
    // something failed, check the error and change stuff in it if needed and rethrow it
  }
};

This works pretty well except for the fact that if I wanted to use expect.assertion the following would not be possible.

expect.assertions(1);
expect(something).myFancyErrorMatcher(somethingElse);

This would fail because jest counts the internal assertion inside myFancyErrorMatcher so the total number of assertions is 2 where to the user of the assertion myFancyErrorMatcher it looks like it should be 1.

@blikblum
Copy link

I'm using snapshot to test generated pdf content and is working fine, but i needed a way to save (and update) the snapshot data as pdf file to easier checking visual changes.

Initially i tried to create a custom matcher but hit the limitation discussed here since i needed to call toMatchSnapshot inside the custom matcher.

A way to expose toMatchSnapshot to my matcher was patch the global expect method and curry the matcher:

const oldExpect = expect
global.expect = function(...args) {
  const expectation = oldExpect(...args)
  expectation.toMatchPdf =expectation.toMatchPdf.bind(null, expectation)
  return expectation
}

I did not tried this route, i preferred to create a wrapper function around toMatchSnapshot checking snapshotState before and after

@SimenB
Copy link
Member

SimenB commented Aug 15, 2018

We actually landed docs for custom snapshot matchers yesterday: #6837

@haffmaestro
Copy link

haffmaestro commented Aug 15, 2018

Takk! @SimenB

Now we just need a nice way to import other matchers :)

FYI if anyone is interested in a semi-hacky way to do this, if your project is already using lodash or underscore, you already have isEqual. Here's how I solved my problem recently:

import isEqual from 'lodash/isEqual';
import diff from 'jest-diff';

const toBeTheThing = (received, expected) => {

    const pass = isEqual(expected, received);

    const message = pass
        ? () => [
            `${this.utils.matcherHint('.not.toBeTheThing')}`,
            'Expected receivedThing to not be the expectedThing:',
            `    ${this.utils.printExpected(expected)}`,
            'Received:',
            `    ${this.utils.printReceived(received)}`,
            ].join('\n')
        : () => {
            const diffString = diff(expected, received, {expand: this.expand});
            return [
                `${this.utils.matcherHint('.toBeTheThing')}`,
                'Expected receivedThing to be the expectedThing:',
                `    ${this.utils.printExpected(expected)}`,
                'Received:',
                `    ${this.utils.printReceived(received)}`,
                'Difference:',
                `    ${diffString}`,
            ].join('\n');
        };

    return {
        pass, message,
    }
};

expect.extend({
   toBeTheThing,
});

@SimenB
Copy link
Member

SimenB commented Aug 15, 2018

You have access to this.equals which is the equality function Jest uses (https://jestjs.io/docs/en/expect#thisequalsa-b)

@haffmaestro
Copy link

Oh, that's super useful!

@RLovelett
Copy link

I expect I am misunderstanding something but this.utils does not seem to provide getObjectSubset and I did expect it to be there.

Is there a way to get this method inside of a custom matcher?

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests