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

feat(expect-expect): support glob patterns for assertFunctionNames (#471) #509

Merged
merged 1 commit into from
Jan 12, 2020

Conversation

folke
Copy link
Contributor

@folke folke commented Jan 10, 2020

Added support for glob patterns to specify assertion function names like request.*.expect (see
micromatch for syntax)

Implements #471

Examples of correct code for working with the HTTP assertions library
SuperTest with the { "assertFunctionNames": ["expect", "request.*.expect"] } option:

/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "request.*.expect"] }] */
const request = require('supertest');
const express = require('express');

const app = express();

describe('GET /user', function() {
  it('responds with json', function(done) {
    request(app)
      .get('/user')
      .expect('Content-Type', /json/)
      .expect(200, done);
  });
});

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a massive fan of having globbing b/c of the extra dependency + people might start expecting it to work everywhere, which'd open up a can of worms for easy footguns, but that's just me being overly paranoid.

Cheers for the PR - lets see what happens :D

(I'll merge later in the day w/ a few other things, so they can be released together)

Copy link
Contributor

@erunion erunion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️This is wonderful.

micromatch is a pretty slim dependency, so overall the plugin still says pretty barebones.

@folke
Copy link
Contributor Author

folke commented Jan 11, 2020

the extra dependency

If you think it would make more sense to create a simple utility function to match wildcards instead of using micromatch, then I could definitely look into that as well.

Ultimately, we don't really need full blown glob pattern matching for this use case. Our import aren't path names, so any functionality related to glob patterns for file paths etc, are not needed.

Imho the only two expression that seem useful are * and **

Example of *

request.get().expect('foo')
request.post().expect('foo')

assertionFunctionNames = ["request.*.expect"]

Example of **

request.get().foo().bar().alpha().whatever().expect('foo')

assertionFunctionNames = ["request.**.expect"]

Both expressions could be easily implemented with a RegExp without depending on micromatch

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 12, 2020

If you think it would make more sense to create a simple utility function to match wildcards instead of using micromatch, then I could definitely look into that as well.

I'd be keen to what you could come up with!

I'm not against using micromatch - my dependency point was a weak one; it's just more about avoiding a new dependency is nice, and globbing tends to be the sort of thing people expect to work everywhere, and so as soon as you add it it has the potential to open a whole can of worms as people assume it's everywhere.

While my main concerns are valid concerns, that's relative to the actual size of the issue, which is very small 🙂


In saying that I'd be very interested in seeing what you could come up with if you want to have a play around!

(I'm going to merge this in the meantime, since we can remove the dependency later)

@G-Rath G-Rath merged commit 295ca9a into jest-community:master Jan 12, 2020
github-actions bot pushed a commit that referenced this pull request Jan 12, 2020
# [23.5.0](v23.4.0...v23.5.0) (2020-01-12)

### Features

* **expect-expect:** support glob patterns for assertFunctionNames ([#509](#509)) ([295ca9a](295ca9a))
* **valid-expect:** refactor `valid-expect` linting messages ([#501](#501)) ([7338362](7338362))
@github-actions
Copy link

🎉 This PR is included in version 23.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@folke
Copy link
Contributor Author

folke commented Jan 12, 2020

@G-Rath
Just finished a small utility function that basically provides the same functionality as needed.

See below:

var assert = require('assert');

function starRegex(pattern: string) {
  return RegExp(
    pattern
      .split('.')
      .map(x => {
        if (x === '**') return '[a-zA-Z\\.]*';
        if (x.includes('**'))
          throw `Double star wildcards can only occur between dots as in request.**.expect`;
        return x.replace('*', '[a-zA-Z]*');
      })
      .join('\\.'),
  );
}

export function starMatch(string: string, pattern: string): boolean;
export function starMatch(string: string, pattern: string[]): boolean;
export function starMatch(string: string, pattern: string | string[]): boolean {
  const patterns = Array.isArray(pattern) ? pattern : [pattern];
  for (const p of patterns) {
    if (starRegex(p).test(string)) return true;
  }
  return false;
}

const test: Array<[string, string, boolean]> = [
  ['request.get.post.expect', 'request.**.expect', true],
  ['request.get.post.expect', 'request.*.expect', false],
  ['request.get.expect', 'request.*.expect', true],
  ['request.get.expect', 'request.**.expect', true],
  ['request.get.expectSomething', 'request.**.expect*', true],
];

for (const [string, pattern, expectedResult] of test) {
  const result = starMatch(string, pattern);
  console.log({
    string,
    pattern,
    regex: starRegex(pattern),
    result,
  });
  assert(result === expectedResult);
}

Outputs:

[
  {
    "string": "request.get.post.expect",
    "pattern": "request.**.expect",
    "regex": /request\.[a-zA-Z\.]*\.expect/,
    "result": true
  },
  {
    "string": "request.get.post.expect",
    "pattern": "request.*.expect",
    "regex": /request\.[a-zA-Z]*\.expect/,
    "result": false
  },
  {
    "string": "request.get.expect",
    "pattern": "request.*.expect",
    "regex": /request\.[a-zA-Z]*\.expect/,
    "result": true
  },
  {
    "string": "request.get.expect",
    "pattern": "request.**.expect",
    "regex": /request\.[a-zA-Z\.]*\.expect/,
    "result": true
  },
  {
    "string": "request.get.expectSomething",
    "pattern": "request.**.expect*",
    "regex": /request\.[a-zA-Z\.]*\.expect[a-zA-Z]*/,
    "result": true
  }
]

Should I create a PR with this change? I'm happy with this approach or using micromatch :)

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 12, 2020

@folke go wild!

Even if it doesn't get merged, I'm keen to check it out, as it looks impressive :D

@folke
Copy link
Contributor Author

folke commented Jan 12, 2020

@G-Rath should I add this to rules/utils.ts? Or in a seperate file like rules/starmatch.ts?

@folke
Copy link
Contributor Author

folke commented Jan 12, 2020

utils.ts seems to be the best location.

Function is pretty small:

/**
 * Checks if node names returned by getNodeName matches any of the given star patterns
 * Pattern examples:
 *   request.*.expect
 *   request.**.expect
 *   request.**.expect*
 */
export function nodeNameMatch(nodeName: string, pattern: string): boolean;
export function nodeNameMatch(
  nodeName: string,
  pattern: readonly string[],
): boolean;
export function nodeNameMatch(
  nodeName: string,
  pattern: string | readonly string[],
): boolean {
  const patterns: string[] = Array.isArray(pattern) ? pattern : [pattern];
  for (const p of patterns) {
    if (
      new RegExp(
        `^${p
          .split('.')
          .map(x => {
            if (x === '**') return '[a-zA-Z\\.]*';
            if (x.includes('**'))
              throw `Double star wildcards can only occur between dots as in request.**.expect`;
            return x.replace('*', '[a-zA-Z]*');
          })
          .join('\\.')}$`,
      ).test(nodeName)
    )
      return true;
  }
  return false;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants