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

Ability to create asymmetric matchers that are recognized by jest. #4711

Closed
mayank23 opened this issue Oct 17, 2017 · 27 comments
Closed

Ability to create asymmetric matchers that are recognized by jest. #4711

mayank23 opened this issue Oct 17, 2017 · 27 comments

Comments

@mayank23
Copy link

mayank23 commented Oct 17, 2017

Hi!

I would like to be able to create asymmetric matchers using an api exported from jest which can automatically insert : $$typeof = Symbol.for('jest.asymmetricMatcher') as a property of the asymmetric matcher.

In turn, I would be able to get support from the pretty-format asymmetric_matcher plugin, and it would be able to print the result of calling myAsymmetricMatcher.toAsymmetricMatcher() for the diff string.

Is this a feature the Jest team would like incorporate?

I can work on a PR if someone could hint at the right place on where to make the changes. I was thinking of adding this jest asymmetric matcher factory possibly in the jest-matcher-utils. Would this be fine?

Thank you!

@cpojer
Copy link
Member

cpojer commented Oct 17, 2017

I'm open to this :)

cc @pedrottimark @thymikee

@pedrottimark
Copy link
Contributor

Although it is implied but not currently documented, Jest assertions evaluate asymmetric matcher objects as defined in Jasmine: https://jasmine.github.io/edge/introduction#section-Custom_asymmetric_equality_tester

Yes, as you suggest, if the assertion fails, the message needs to display an expected value better than {"asymmetricMatch": [Function asymmetricMatch]}

@mayank23 Here are some files to study, especially as you plan ahead for tests to add:

When I studied the code to answer your question about toAsymmetricMatcher method, my first impression is to call the method with arguments especially printer

  • to move logic about built-in matchers from plugin to their classes (reduce coupling)
  • allow application-specific matcher to serialize its criteria

@cpojer Can you confirm or adjust this direction:

  • Add a new method of expect as we did for addSnapshotSerializer
  • Make changes in expect package instead of jest-matcher-utils

@cpojer
Copy link
Member

cpojer commented Oct 17, 2017

@pedrottimark yep! Sounds great.

@mayank23
Copy link
Author

@pedrottimark @cpojer Awesome, thanks for the information! Will start on this soon.

@pedrottimark
Copy link
Contributor

pedrottimark commented Oct 17, 2017

@mayank23 Super. Can you link from a comment in this issue to a gist of some examples of asymmetric matchers with received and expected values so an assertion would fail?

That will help me double-check any improvement to interface between matcher instances and plugin to print expected value so EDIT expect and jest-diff display clear information.

@mayank23
Copy link
Author

mayank23 commented Oct 18, 2017

Hi @pedrottimark , here's my gist of 3 examples.

https://gist.github.com/mayank23/4a5ac7a7cbb745aefc995777519338f5

I listed what I think would be nice to have printed when a match fails in each Asymmetric Matcher's getAsymmetricMatcher method.

Technically, this should run if all files were in a single directory, I commented the main use case for each too.

The main test cases file: https://gist.github.com/mayank23/4a5ac7a7cbb745aefc995777519338f5#file-testcases-js. Each test case name is denoted with the prefix 'PASS:' and 'FAIL:' for the passing and failing test cases respectively.

Thanks!

@mayank23
Copy link
Author

Hi @pedrottimark did you have any questions regarding the examples I links? Thanks!

@pedrottimark
Copy link
Contributor

@mayank23 You gave just what I need to stretch my own thinking. Thank you very much.

This issue will be in my mental slow cooker until Friday.

In case this helps to stretch your thinking about the developer experience we are aiming for, here is a picture of results from jest-diff when one of the built-in asymmetric matchers fails.

diff_objectcontaining

@pedrottimark
Copy link
Contributor

pedrottimark commented Oct 18, 2017

Forgot to ask if you can suggest any patterns you have seen to name a factory function?

EDITED AGAIN on 2017-10-28: To explore the space of possible solutions:

  • expect.AsymmetricMatcher base class so test code can extend it or construct from it
  • provide a function that given asymmetric match object, adds $$typeof: Symbol.for('jest.asymmetricMatcher') property, and returns mutated object

@pedrottimark
Copy link
Contributor

Something out of scope for this issue, but that we want not to make harder to do in the future:

In some cases like picture in preceding comment, results from jest-diff would be more useful if they could distinguish which asymmetric match sub-tests passed or failed.

@pedrottimark
Copy link
Contributor

pedrottimark commented Oct 20, 2017

@thymikee @mayank23 First draft of contract between matchers and plugin for y’all to critique:

EDITED on 2017-10-25

For plugin to get name of matcher:

  • If matcher has getName() method, return its value
  • Else if val.constructor && val.constructor.name neither undefined nor Object (for class)
  • Else use string EDIT: asymmetricMatcher (for plain object as described in Jasmine docs)

EDITED on 2017-10-28 For plugin to serialize matcher:

  • If matcher has serialize method call with same args as plugin serialize method except omit val because matcher decides which of its values to print
  • Else if matcher has getArgs() method, plugin uses the returned array instead of current val.sample to enclose in parentheses following name of matcher. If more than one arg and at least one arg serialized to multiple lines, then plugin calls internal helper printListItems to serialize on separate lines (as if a function by prettier). Otherwise it joins with comma-space the args mapped using printer callback function.
  • Else name of matcher. Because args are not known, enclose the name in square brackets?

EDITED: serialize() replaces toAsymmetricMatcher() to override default serialization:

serialize(/* ignores its arguments in this example */) {
  return 'any(' + fnNameFor(this.sample) + ')';
}
Baseline minified Proposed minified looks like assertion
Anything anything()
Any<Number> any(Number)
ArrayContaining ["Alice", "Bob"] arrayContaining(["Alice", "Bob"])
ObjectContaining {"x": Any<Number>, "y": Any<Number>} objectContaining({"x": any(Number), "y": any(Number)})
StringContaining "JavaScript" stringContaining("JavaScript")
StringMatching /\d{3}-\d{4}/ stringMatching(/\d{3}-\d{4}/)

Withdraw special case for arg of 2 built-in matchers because cannot do it for application-specific.

arrayContaining(Array [
  "Alice",
  "Bob",
])

objectContaining(Object {
  "x": any(Number),
  "y": any(Number),
})

Hypothetical examples from gist linked in earlier comment:

  • GreaterThan without toString nor toAsymmetricMatch but getArgs() { return [this.expected]; } would serialize as for example: GreaterThan(1) or GreaterThan("a")
  • ActionTypeMatching might be an example of getString method:
    • To print key and name of actions file if import internally as in gist
    • To print only the key, if its constructor takes key and possibly large action types object.
  • TranslationKeyMatching because of translations seems similar to action types

@pedrottimark
Copy link
Contributor

pedrottimark commented Oct 25, 2017

@thymikee Indirectly related to this issue, if Jest raises awareness of built-in asymmetric matchers so people (like me :) study or even copy-and-edit to make application-specific matchers, can we converge on one pattern for where matchers throw error about invalid sample?

3 in constructor function:

2 in asymmetricMatch method:

EDITED on 2017-10-28 to see developer experience from-the-outside-in:

For any which throws for undefined arg in constructor:

  • construct matcher in assertion: TypeError: any() expects to be passed a constructor function. Please pass one or use anything() to match any object.
  • construct matcher in describe preceding two test whose assertions refer to it: encountered a declaration exception and so on
  • construct matcher at module scope: Test suite failed to run and so on

For arrayContaining which throws in asymmetricMatch method You must provide an array to ArrayContaining, not 'string'. from each test:

  • construct matcher in assertion
  • construct matcher in describe preceding two test whose assertions refer to it
  • construct matcher at module scope

Therefore, throw error about args in constructor so line number points to source of problem?

The asymmetricMatch method still might throw some errors like arg is not a constructor.

@pedrottimark
Copy link
Contributor

pedrottimark commented Oct 28, 2017

@mayank23 After thinking about this at turtle pace, here is hint where to make changes:

@cpojer @thymikee What do you think about adding AsymmetricMatcher class to expect API?

To get $$typeof property which the plugin tests when an assertion fails:

  • in ES2015 and later: class MyMatcher extends AsymmetricMatcher as built-in matchers
  • in ES5: const myMatcher = new AsymmetricMatcher() and then set properties, or:
// EDITED to add `getExpectedType` and replace ES2015 arrow functions with functions :)
const myMatcher = Object.assign(new AsymmetricMatcher(), {
  asymmetricMatch: function (received) { return received % modulus === 0; },
  getExpectedType: function () { return 'number'; },
  getName: function () { return 'modulusMatcher'; },
  getArgs: function () { return [modulus]; },
});

@thymikee
Copy link
Collaborator

I'm good with exposing AsymmetricMatcher class and moving the sample type check to the constructor 👍.

@mayank23
Copy link
Author

Alright, will get started on this soon. Thanks!

@pedrottimark
Copy link
Contributor

@mayank23 Super! To keep a short feedback loop of review, I suggest:

While you work on code, would you like me to think about docs?

@mayank23
Copy link
Author

mayank23 commented Oct 29, 2017

awesome, thanks for the advice! yes that would be great!

@pedrottimark
Copy link
Contributor

@mayank23 Happy New Year! Do you still want to make a pull request? Is it possible in January?

@mayank23
Copy link
Author

mayank23 commented Jan 8, 2018

Hi @pedrottimark Happy new year to you too! Yes I will try to get this in this month. Sorry about the delay

@aymericbouzy
Copy link
Contributor

I'm a total newbie but interested in helping if someone guides me around!

@pedrottimark
Copy link
Contributor

@aymericbouzy Welcome to Jest! I will read your recent issue and review this issue on Friday.

Here are some first steps:

  1. Read https://github.com/facebook/jest/blob/master/CONTRIBUTING.md
  2. Fork the Jest repo
  3. Clone your fork to your computer
  4. Follow contributing instructions under Workflow including yarn install and yarn test

Bonus: If you find anything in CONTRIBUTING.md that is incorrect or unclear, make a note for possible future pull request :)

@aymericbouzy
Copy link
Contributor

Hi @pedrottimark! Thanks 😃 I've done all that alright 👍 Not exactly sure what you mean by Workflow, but I've run both commands successfully.

I must say the experience of opening the repo in VS Code and having the recommended extensions showing up is impressive 🤩 Especially the Jest extension looks incredible !

@aymericbouzy
Copy link
Contributor

mh that's weird, when jest extension runs from VS Code, there are a lot of problems with the test snapshots, it says almost all of them have changed 🤔
Going back to running the tests from my terminal.

@SimenB
Copy link
Member

SimenB commented Mar 8, 2018

#5517

@SimenB SimenB closed this as completed Mar 8, 2018
@yiochen
Copy link

yiochen commented Nov 13, 2018

Can we reopen this issue? I don't think the issue is being addressed. We still cannot provide a good error message other than

Expected value to equal:
      {"asymmetricMatch": [Function asymmetricMatch]}

when our custom asymmetric matcher returns false

@SimenB
Copy link
Member

SimenB commented Nov 14, 2018

Please open up a new issue with a reproduction. Thanks!

@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.
Projects
None yet
Development

No branches or pull requests

7 participants