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(unbound-method): create rule #765

Merged
merged 12 commits into from
Mar 13, 2021
Merged

feat(unbound-method): create rule #765

merged 12 commits into from
Mar 13, 2021

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Feb 20, 2021

This is an initial attempt at creating an "jest aware" version of @typescript-eslint/unbound-method that won't report on passing "unbound" methods to expect.

For now I've copied the rule and its tests (including fixtures) over verbatim and then kept modifications to a minimum so that it's easier to keep up to date with the base rule. (ideally would be nice if we could watch the base rule for changing some how 🤔).

It turns out that the logic is actually pretty straightforward, at least apparently to catch the common cases, but for the same reason I suspect I've missed an edge-case or two.

Aside from "what edge cases have I missed", my primary question right now is what should we do if we can't get the type service?

Personally, I'd really like to be able to enable this rule without having to worry about if the TypeScript service is available, as that'll make it a lot easier for sharable configs. However, the default behaviour is to throw saying that you need type information and its possible that people could be confused that the rule isn't do anything (however I think this'll be less likely for us, as the rule implies some exposure to the base rule and therefore TypeScript).

If we think the rule should error when type information is not available, I think it would be good to add an option to allow choosing between throwing an error or silently doing nothing, which could be used in shareable configs to safely enable the rule without having to worry.

I've created a test that hopefully captures the behaviour of the rule when running without the TypeScript service, but I could be completely wrong on that.

(I have just realised the rule actually imports ts for some enum values, so will have a think about this - we could drop that dependency easily by just inlining the enums and making it a type import 🤔)

@bradzacher would be good to have your input :)

Closes typescript-eslint/typescript-eslint#2951

@G-Rath G-Rath requested a review from SimenB February 20, 2021 22:49
@G-Rath
Copy link
Collaborator Author

G-Rath commented Feb 20, 2021

I.e. it could be taught that expect(clazz.method).toBeTruthy() is safe, but expect(clazz.method).toThrow() is unsafe.

(will implement this now) implemented

@G-Rath G-Rath force-pushed the create-unbound-rule branch 6 times, most recently from 475d256 to dc2f4dc Compare February 21, 2021 01:29
Copy link
Contributor

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

if the only change you're making it to make this not report for expect calls, you can simplify this as an extension rule.

import TSESLintPlugin from '@typescript-eslint/eslint-plugin';
const baseRule = TSESLintPlugin.rules['unbound-method'];

// ...

export default util.createRule<Options, MessageIds>({
  ...baseRule,
  create(context) {
    const baseSelectors = baseRule.create(context);
    return {
      ...baseSelectors,
      CallExpression(node: TSESTree.CallExpression): void {
        inExpectCall = isJestExpectCall(node);
      },
      'CallExpression:exit'(node: TSESTree.CallExpression): void {
        if (inExpectCall && isJestExpectCall(node)) {
          inExpectCall = false;
        }
      },
      MemberExpression(node: TSESTree.MemberExpression): void {
        if (inExpectCall) {
          return;
        }
        baseRule.MemberExpression(node);
      },
    };
  },
})

Note that: this does create a dep on ts-eslint plugin.

You could make this safer and not add a direct dependency on the plugin with something like:

const baseRule = (() => {
  try {
    const TSESLintPlugin = require('@typescript-eslint/eslint-plugin');
    return TSESLintPlugin.rules['unbound-method'];
  } catch (e) {
    return null;
  }
})();

export default util.createRule<Options, MessageIds>({
  ...baseRule ?? { /* some defaults */ },
  create(context) {
    if (!baseRule) { return {} }
    return { ... };
  },
})

@G-Rath
Copy link
Collaborator Author

G-Rath commented Feb 21, 2021

you can simplify this as an extension rule

That's a really good point! It's a lot cleaner to do it that way (thanks for the snippets btw).

I'd be happy with doing it that way, using the try/catch to avoid the hard dependency and make it an optional peer dependency to keep package managers happy.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Feb 21, 2021

ugh while I like the idea, re-writing this as an extension rule is proving hard to test because I can't re-require the actual rule to cover it's behaviour when the package "is not there" (aka throws an error):

describe('when @typescript-eslint/eslint-plugin is not available', () => {
  let rule = require('../unbound-method').default;

  beforeEach(() => {
    jest.resetModules();
    jest.mock('@typescript-eslint/eslint-plugin', () => {
      console.log('require');
      return () => {
        throw new Error();
      };
    });
    rule = require('../unbound-method').default;
  });

  new ESLintUtils.RuleTester({
    parser: '@typescript-eslint/parser',
    parserOptions: {
      sourceType: 'module',
      tsconfigRootDir: rootPath,
      project: './tsconfig.json',
    },
  }).run('unbound-method jest edition without type service', rule, {
    // valid: validTestCases.concat(invalidTestCases.map(({ code }) => code)),
    valid: invalidTestCases.map(({ code }) => code),
    invalid: [],
  });
});

This is what I've been hacking together, which fails because you can't have RuleTester in an it (as it creates its from the given test cases), but beforeEachruns for eachit` statement - so by the time the tester runs, the require has already happened :/

@SimenB what are you thoughts on what we should cover for this testing wise?

@G-Rath
Copy link
Collaborator Author

G-Rath commented Feb 21, 2021

(in classic fashion, 5 minutes after my last comment, I glanced at some of the test code for eslint-plugin-eslint-config, which has similar tests, and realised how I should be able to test this 🤦)

@G-Rath
Copy link
Collaborator Author

G-Rath commented Feb 21, 2021

I've left the original fixtures + tests in there for now, but I've gotten full cover with minimal hacking and without blowing out the run time of the test suite.

The only issue is that apparently there's improper teardown going on:

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks.

I think maybe the TestRunner is staying alive longer than it should because the rule is erroring? Not sure why though 🙁

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 6, 2021

@SimenB would be keen to hear your thoughts on this - aside from the doc tweaks, this rule should be good to go.

I've also got a few ideas on some more type-based rules, one of which I've already got a semi-working implementation for (checking that .toThrow & co matchers are passed a function to be called).

Base automatically changed from master to main March 6, 2021 19:29
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I'm a fan of type based rules 👍 I don't think they should go in recommended, but we can have a different rule set with type based ones

src/rules/unbound-method.ts Outdated Show resolved Hide resolved
src/rules/unbound-method.ts Outdated Show resolved Hide resolved
src/rules/unbound-method.ts Show resolved Hide resolved
@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 6, 2021

Awesome! I'll look to continue going 🎉

I'm updating the docs now, and then will address your changes - hopefully it should be out of draft sometime today.

@G-Rath

This comment has been minimized.

@G-Rath G-Rath force-pushed the create-unbound-rule branch 2 times, most recently from c596f96 to 65a1a10 Compare March 6, 2021 20:36
@G-Rath

This comment has been minimized.

@G-Rath G-Rath marked this pull request as ready for review March 6, 2021 21:10
@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 6, 2021

I think we're good to go - @SimenB I'll leave this around for a bit in case you want to review the docs and changes; feel free to merge if you're happy and want to get it out, otherwise I'll look to do so sometime today or tomorrow :)

<!-- end rules list -->
<!-- end base rules list -->

## TypeScript Rules
Copy link
Member

Choose a reason for hiding this comment

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

something about needing to install the upstream plugin as well? Possibly in the table if we'll have othe rrules in the future that do not use type info

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly in the table if we'll have othe rrules in the future that do not use type info

You mean if we have other rules that don't extend base rules.


You're completely right - I don't expect us to make any more rules that extend base rules, only more rules that use type information; so I might leave the table as is for now and just add a sentence saying that unbound-method is an extension rule (as well as updating its own docs).

This was referenced Mar 17, 2021
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.

[unbound-method] support an allowlist
3 participants