search

jest-community / eslint-plugin-jest

ESLint plugin for Jest
MIT License
1.13k stars 234 forks source link

Rewrite in TypeScript #256

Closed SimenB closed 5 years ago

SimenB commented 5 years ago

Now that https://github.com/typescript-eslint/typescript-eslint/pull/425 has been released, it should be relatively straightforward to migrate.

I think porting one and one rule makes the most sense. Feel free to pick one šŸ™‚

G-Rath commented 5 years ago

I've done a first pass of no-mocks-import, which passes all the tests.

So far, it seems to me like this'll be a bit teeth pully, as ts-estree doesn't provide you with lots of nice type guards like TS does - as a result, I've started to make my own in tsUtils, but am trying to be conservative for now, since I don't know ts-estree well enough to predict how wild things might get.

I've also encapsulated the actual primary condition into a isMockImportLiteral function - let me know if you're happy with how I've done that; I've never worked in the FB/jest domain, so I don't know what you guys prefer for coding in general :joy:

Finally, off the bat there's already room for improvement, which ties in to everything I've said so far: right now the rule is only checking actual string literals, which means template strings already won't work right off the bat (regardless of if they've got actual embedded expressions or not)

This could be improved to support them w/o expressions, but you could also take this further to try and provide basic support for expressions, since you might be able to do a basic search of surrounding nodes in an attempt to see if the expression is a constant, and if it is evaluate its value.

All in all, for now I've opted for doing the basics, w/ the idea that people can fill issues requesting improvements, just to keep things simple.

I've not made a PR since I saw your previous PRs were reverted following some breaking of peoples builds. Let me know if you're still wanting to go ahead with converting to TS.

For now I'm going to continue trying to pick off the easy rules - let me know any thoughts you have :)

jeysal commented 5 years ago

@G-Rath thanks for investing time in this! What you've described sounds fine to me. @SimenB will probably want to kick off a second attempt at TS migration at a less busy time

SimenB commented 5 years ago

Thanks @G-Rath! Yeah, it seems using the module from https://github.com/typescript-eslint/typescript-eslint/pull/425 requires consumers to use typescript as well, so I need to figure out how we can author rules in TS without consumers noticing at all. I'll try to get to this, but I'm still swamped at work... Feel free to dig into it if you don't mind!

G-Rath commented 5 years ago

I don't know enough about the eslint ecosystem to know if its worth using the @typescript-eslint/parser over the current one, which is now my question - what is the advantage?

I'm happy to help write TS, regardless of what it's for, but yeah I think the decision that needs to be made is about converting to TS vs switching to the @typescript-eslint/parser.

The former could be started on right now if you wanted, but as you've found out the latter is a breaking change, and thus comes down to pros vs cons which I obviously can't decide for you :)

SimenB commented 5 years ago

I don't know enough about the eslint ecosystem to know if its worth using the @typescript-eslint/parser over the current one, which is now my question - what is the advantage?

We don't want to use the parser in our own rules - we want to use the utils which provide way better types than the ones from DefinitelyTyped as well as some nice utils for options etc. It's also stricter (also called opinionated šŸ˜›), forcing us to follow some good conventions (fixing both #201 and #202 at once).

We do however want to use the parser for linting our own code, but that should not be visible to consumers.

The dependency on typescript was not on purpose, see https://github.com/typescript-eslint/typescript-eslint/pull/425#issuecomment-498302492

When a release is out, we can probably retry this (I'll revert my earlier revert). Will need to dig into #268 as well at that point.

Mark1626 commented 5 years ago

@SimenB I'll pick up a rule, can you tell me where code migrated to typescript is maintained.

SimenB commented 5 years ago

A new try is up here: #305.

@M4rk9696 thanks! A PR to that branch (reapply-ts) is most welcome. Or if I'm able to land it, to master

SimenB commented 5 years ago

@G-Rath would you be able to PR no-mocks-import to the reapply-ts branch of this repo? šŸ™‚

G-Rath commented 5 years ago

@SimenB I can but try ;P

Give me a few minutes (or an hour, depending on the work - I've not actually been following the activity šŸ˜¬)

SimenB commented 5 years ago

There is absolutely no rush šŸ™‚

G-Rath commented 5 years ago

@SimenB done in #309

It took a bit longer than a few minutes b/c of a standup meeting & having to update the code a bit (but nothing major).

All the tests are passing, so I think it's fine :)

G-Rath commented 5 years ago

I've been doing everything in a single generic branch, and have about a quarter of the rules converted.

My plan is to just cherry-pick at the end. There are a couple of that don't require tsUtils that I can cherry-pick, which I'll try and do now.

Otherwise, I'll finish off the rest off the rules this evening/weekend :)

Sorry for all the failing tests - I'll check them out once I'm home, as I'm rushing to catch a train šŸ˜¬ I have no idea what's up w/ the one about the git reference, and so don't know how to fix it either. I'm pretty sure it's related to my fork & updating it? Either way, the code is there.

SimenB commented 5 years ago

Oh wow @G-Rath, that's awesome!

Don't worry about the commitlint check on CI - it's confused by the merge commit travis creates. As long as lint and test (with coverage) passes, you're good šŸ™‚

G-Rath commented 5 years ago

@SimenB just a heads up that the meta property should be checked on all my PRs; I've been forgoing trying to figure out what the best values for the description, setting the correct "recommended" value, the "category", and the "type" in favor of converting the actual rules as I feel those are somewhat arbitrary.

I've been just copying the header for each rule as the description :)

I've also not tested any of the fixes - I think they'll work fine but the only thing I'm worry about is what happens if you return an empty array..? I've not written eslint plugins before so don't know what the best practices are.

I'm happy to follow your lead on any changes you want made.

SimenB commented 5 years ago

Ah, ok, good to know. I'll verify

The fixes are all covered by unit tests, so if the tests pass then we're good. If a violation is not fixable, we should not return a fixer (that way the "n error potentially fixable" message from eslint is not misleading)

G-Rath commented 5 years ago

I think we've nearly gotten all the low hanging fruit, so I'm going to look to tackle the converting of the exportCase & other type guard util functions (like method, method2, etc).

I've actually already done exportCase, but I've not pushed it yet b/c I need to explore them a bit more, as some of them are definitely guards, while others are not.

Once that's done, we'll be able to tackle all of the prefer-*, as they're the main rules that consume those methods.

I've got a few more rules to convert & cherry pick first tho - I'm catching a flight in a day or so, so I might go dark for a bit, but at this rate I'd say we should easily be done by the end of month, if not by next week :D

SimenB commented 5 years ago

Haha, this has been awesome, thank you so much for your help!

SimenB commented 5 years ago

TS ESLint had a stable release, so I merged the PR to master. Also updated OP

G-Rath commented 5 years ago

I merged the PR to master

Well, this went about as well as I expected - I woke up to about 20 emails for this repo, half of which being semantic bot :joy:

but seriously it actually did go as expected: the bugs I've seen are pretty much what I expected, and the fixing PRs have been great, and will make everything more stable.

I'll look to power through getting the rest of the rules converted over the next couple of days. Pretty much all the rules that are left to do I've got half converted, as I'm picking the ones w/ the least amount of imports from tsUtils first.

SimenB commented 5 years ago

Heh, yeah... It wasn't too bad, though!

G-Rath commented 5 years ago

@SimenB Just wanted to let you know I've not forgotten about this šŸ˜‰

I've got 2 rules left to convert, and the new guards + expect parser is working a treat.

Life got a bit busy once I got back, but I hope to have a PR for review by midweek.

As an example of how nice the new expect stuff is: prefer-to-be-null & prefer-to-be-undefined took literally 5 minutes to convert, despite being the two rules that used those massive expect guards, as they were having to account for not.

Now they're just:

const isNullEqualityMatcher = (
  matcher: ParsedExpectMatcher,
): matcher is ParsedNullEqualityMatcher =>
  EqualityMatcher.hasOwnProperty(matcher.name) &&
  matcher.node.parent !== undefined &&
  matcher.node.parent.type === AST_NODE_TYPES.CallExpression &&
  hasOneArgument(matcher.node.parent) &&
  isNullLiteral(matcher.node.parent.arguments[0]);

/* in create: */
if (!isExpectCall(node)) {
  return;
}

const { matcher } = parseExpectCall(node);

if (matcher && isNullEqualityMatcher(matcher)) {
  context.report({
    fix(fixer) {
      return [
        fixer.replaceText(matcher.node.property, 'toBeNull'),
        fixer.remove(matcher.node.parent.arguments[0]),
      ];
    },
    messageId: 'useToBeNull',
    node: matcher.node.property,
  });
}
SimenB commented 5 years ago

This is super exciting šŸ˜€

G-Rath commented 5 years ago

Except I missed the fact that you can chain modifiers (i.e resolve.not) šŸ˜­

Not a big deal since we only really check for them in valid-expect, but means I have to code up a linked-list style loop to the parser which are always a bit tedious in TS

SimenB commented 5 years ago

You can only have expect().not.toBe(), expect().resolves.toBe(), expect().rejects.toBe(), expect().resolves.not.toBe() and expect().rejects.not.toBe(). So I don't think we necessarily need a linked list? If we can say a parsed expect may or may not be not, and it might be either resolves or rejects. Dunno, might make it hard to do .parent etc?

G-Rath commented 5 years ago

Interesting. The typescript types allow it, but it fails at runtime: http://puu.sh/DYIn4/7278eb6827.png

When I say "linked list" it's not anything complex, just:

export const parseExpectCall = (expectCall: ExpectCall): Expectation => {
  const expectation: Expectation = { expectCall };

  if (!isExpectMember(expectCall.parent)) {
    return expectation;
  }

  let node = expectCall.parent;
  let previousModifier: { modifier?: ParsedExpectModifier } = expectation;

  while (node.parent && isExpectMember(node.parent)) {
    const member = parseExpectMember(node.parent);

    if (!isParsedExpectModifier(member)) {
      expectation.matcher = member;
      break;
    }
    previousModifier = previousModifier.modifier = member;

    node = node.parent;
  }

  return expectation;
};

It was more a bit of a shock b/c I'd not considered it, and so it wasn't in the output šŸ˜¬ Doing it this way means I don't have to refactor anything else, and also has the bonus of requiring the previous modifier to be defined (otherwise parsedExpect.modifer !== undefined && parsedExpect.modifier2 !== undefined)

SimenB commented 5 years ago

That should probably be a type error, yeah. The Jest team doesn't maintain those typings, but Jest's internal typings are probably wrong in this regard as well. I'll make a point to fix it before releasing Jest 25

G-Rath commented 5 years ago

Ahh I see how it is - I had a proper read over everything, and yeah that loop while nice is overkill (plus the types are murder):

  describe('each', () => {
    it.each([
      expect(true).not.resolves,  // true
      expect(true).not.rejects,    // true
      expect(true).resolves.not, // false
      expect(true).rejects.not    // false
    ])('should be undefined', (result) => {
      expect(result).toBe(undefined);
    });
  });

So the allowed modifiers are:

not resolves rejects resolves.not rejects.not

However, valid-expect doesn't actually check for that. In fact, it has those in its tests as valid. In fact, the majority of its not valid tests have not first.

I'll avoid "fixing" that if possible currently, and instead look into it after I've gotten the remaining rules converted & merged.

But in light of this I think the typings can be a lot simpler. I have a few ideas on a couple of different ways it could be typed, and will have a play around to find out what works best :)

G-Rath commented 5 years ago 
------------------------------|----------|----------|----------|----------|-------------------|
File                          |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------------|----------|----------|----------|----------|-------------------|
All files                     |    96.26 |    92.62 |    89.54 |     96.4 |                   |
 src                          |      100 |      100 |      100 |      100 |                   |
  index.js                    |      100 |      100 |      100 |      100 |                   |
 src/processors               |      100 |      100 |      100 |      100 |                   |
  snapshot-processor.js       |      100 |      100 |      100 |      100 |                   |
 src/rules                    |    96.21 |    92.55 |    89.26 |    96.35 |                   |
  consistent-test-it.ts       |      100 |      100 |      100 |      100 |                   |
  expect-expect.ts            |      100 |      100 |      100 |      100 |                   |
  expectGuards.ts             |    72.73 |    47.27 |    53.33 |    72.73 |... 17,424,425,450 |
  final.ts                    |    92.54 |    83.87 |       84 |    92.42 |... 43,365,367,433 |
  lowercase-name.ts           |      100 |      100 |      100 |      100 |                   |
  no-alias-methods.ts         |      100 |      100 |      100 |      100 |                   |
  no-commented-out-tests.ts   |      100 |      100 |      100 |      100 |                   |
  no-disabled-tests.ts        |      100 |      100 |      100 |      100 |                   |
  no-duplicate-hooks.ts       |      100 |      100 |      100 |      100 |                   |
  no-empty-title.ts           |      100 |      100 |      100 |      100 |                   |
  no-export.ts                |      100 |      100 |      100 |      100 |                   |
  no-focused-tests.ts         |      100 |      100 |      100 |      100 |                   |
  no-hooks.ts                 |      100 |      100 |      100 |      100 |                   |
  no-identical-title.ts       |      100 |      100 |      100 |      100 |                   |
  no-if.ts                    |      100 |      100 |      100 |      100 |                   |
  no-jasmine-globals.ts       |      100 |      100 |      100 |      100 |                   |
  no-jest-import.ts           |      100 |      100 |      100 |      100 |                   |
  no-large-snapshots.js       |      100 |      100 |      100 |      100 |                   |
  no-mocks-import.ts          |      100 |      100 |      100 |      100 |                   |
  no-standalone-expect.ts     |      100 |      100 |      100 |      100 |                   |
  no-test-callback.ts         |      100 |      100 |      100 |      100 |                   |
  no-test-prefixes.ts         |      100 |      100 |      100 |      100 |                   |
  no-test-return-statement.ts |      100 |      100 |      100 |      100 |                   |
  no-truthy-falsy.ts          |      100 |      100 |      100 |      100 |                   |
  no-try-expect.ts            |      100 |      100 |      100 |      100 |                   |
  prefer-called-with.ts       |      100 |      100 |      100 |      100 |                   |
  prefer-expect-assertions.ts |      100 |      100 |      100 |      100 |                   |
  prefer-inline-snapshots.ts  |      100 |      100 |      100 |      100 |                   |
  prefer-spy-on.ts            |      100 |      100 |      100 |      100 |                   |
  prefer-strict-equal.ts      |      100 |      100 |      100 |      100 |                   |
  prefer-to-be-null.ts        |      100 |      100 |      100 |      100 |                   |
  prefer-to-be-undefined.ts   |      100 |      100 |      100 |      100 |                   |
  prefer-to-contain.ts        |      100 |      100 |      100 |      100 |                   |
  prefer-to-have-length.ts    |      100 |      100 |      100 |      100 |                   |
  prefer-todo.ts              |      100 |      100 |      100 |      100 |                   |
  require-tothrow-message.ts  |      100 |      100 |      100 |      100 |                   |
  tsUtils.ts                  |    89.86 |    75.38 |    73.08 |    89.71 |... 42,145,166,178 |
  util.js                     |       65 |        0 |    22.22 |    70.59 |     7,10,15,20,29 |
  valid-describe.ts           |      100 |      100 |      100 |      100 |                   |
  valid-expect-in-promise.ts  |      100 |      100 |      100 |      100 |                   |
  valid-expect.ts             |      100 |    98.75 |      100 |      100 |               274 |
------------------------------|----------|----------|----------|----------|-------------------|

Guess who got valid-expect typesafe last night ;)

~I've just got no-large-snapshots to convert, which will be interesting b/c it uses babel-eslint - hopefully it'll have TS typings, but if not I might have to leave that one to you @SimenB~

I've now finished no-large-snapshots, resulting in babel-eslint not being needed anymore, as all the tests have been merged into the RuleTester (meaning the .snap is also gone).

I expect to have a PR sometime this weekend šŸ˜„

(of course after saying that I realised I should re-review the guards for tests, hooks & describe :joy:)

SimenB commented 5 years ago

This is done! Thank you to everyone who contributed, and especially @G-Rath who did all the hard work šŸŽ‰