feat: support `@jest/globals` #1094

G-Rath · 2022-04-25T19:47:47Z

I think the core logic is pretty much done - just need to shore it up with tests (including coverage for TS-specific imports), switch over all the rules to use it, and implement support for require.

I'm considering landing this in two parts: 1. just switching us over to checking if references are globals, then 2. (re)implementing the special casing of imports from @jest/globals, since they are really two different features and that might make it a bit nicer to list in changelogs etc; but will see how the PR ends up looking.

Resolves #556

SimenB · 2022-04-25T20:00:01Z

Yay! Will review tomorrow (currently at a concert) if you want @G-Rath. I see it's draft and red ci, so might not be ready for review?

G-Rath · 2022-04-25T20:05:24Z

Yeah it's not quite ready for a complete review - I tagged you so you knew it was coming incase you were planning on doing a new major soon (since Jest v28 is out) and we spoke about dropping the jest globals from recommended once we support this; + you might have some ideas/snippets for getting complete coverage (e.g. ref.defs is defined as an array, though I'm pretty sure it should always have 0 or 1 elements, but there might be a edge-case I've forgotten about 🤷).

Now that I know you've seen it, I'll remove your review and re-add once it is actually ready for review 😄

SimenB · 2022-04-25T20:09:59Z

Haha, sounds good!

SimenB · 2022-04-25T20:10:54Z

(no offense, but a Tool concert is more fun than code 🤘)

G-Rath · 2022-04-28T22:49:41Z

@SimenB I'm getting some weird behaviour when trying to generate a coverage report for utils.ts - if I do yarn run test utils --coverage, it seems to be generating a report with the files in coverage/lcov-report/rules/..., and the actual content of coverage/lcov-report/rules/utils.ts.html has this error:

Cannot read property 'start' of undefined
TypeError: Cannot read property 'start' of undefined
    at /c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-reports/lib/html/annotator.js:177:41
    at Array.forEach ()
    at annotateBranches (/c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-reports/lib/html/annotator.js:128:33)
    at annotateSourceCode (/c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-reports/lib/html/annotator.js:252:9)
    at HtmlReport.onDetail (/c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-reports/lib/html/index.js:414:33)
    at LcovReport. [as onDetail] (/c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-reports/lib/lcov/index.js:28:23)
    at Visitor.value (/c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-lib-report/lib/tree.js:38:38)
    at ReportNode.visit (/c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-lib-report/lib/tree.js:88:21)
    at /c/Users/G-Rath/workspace/projects-oss/eslint-plugin-jest/node_modules/istanbul-lib-report/lib/tree.js:92:19
    at Array.forEach ()

If I run the whole test suite with coverage (yarn run test --coverage) then the report is fine, with the contents in coverage/lcov-report/src/....

I can reproduce this on main - could you see if happens for you too? and if so what'd be the best way to tackle this - should I make a new issue on the jest repo? I don't know if the issue is actually with jest, istanbul or something else and I don't know if it happens on Jest v28 either.

SimenB · 2022-04-29T06:23:05Z

The changing path is jestjs/jest#7176, would be great to fix that.

Unsure of the error, tho...

G-Rath · 2022-04-30T01:51:46Z

Ok I think this should ready for review - couple of things:

Currently this is just about answering the question of "is this a jest function?", not "what jest function is this?" - that means e.g. import { xit as it } from '@jest/globals'; is correctly determined to be a jest function, but rules will treat it as it rather than xit
- All of the info for supporting this is present, we just need to change the function signatures of the utils to be something like parseIfJestFunction which returns JestFunction | null instead of a boolean
- ~~A small part of me wonders if we should maybe not do this, as it could serve as a way of bailing out~~ we should do it, because this is a core feature of ESM imports + you can "bailout" by just assigning to a variable
Similarly, we don't attempt to resolve variables e.g. const it = jest.it won't work
- this is something I think we should support less, but at the same time I actually don't think it'll be as hard as it might first seem (we're already doing this kind of logic in valid-expect-in-promise - it'd mainly just be a huge refactor)
- this means we don't support jest.* (and import jest from '@jest/globals'), which I think we will need to support at some point because of other jest methods that we currently don't run through this new logic (e.g. jest.spyOn)
This PR also re-writes isHook to work the same way as isTestCaseCall and isDescribeCall - that could easily be cherry-picked into another PR though

Having written all that up, I think really this is only half of the work required to really close #556, because supporting @jest/globals should also mean we handle other jest methods like jest.spyOn and jest.mock.

This actually makes me think it might be best to go with my original plan of trying to land support for "true" global scope/reference first, and then follow it up with adding support for @jest/globals as an exception to that logic.

@SimenB what do you think? I'm happy to split this up into a couple of different PRs and also land it via next (even if it's not a breaking change, having a few changelog entries could be good).

G-Rath · 2022-04-30T01:59:29Z

Having written all that up, I think really this is only half of the work required to really close #556, because supporting @jest/globals should also mean we handle other jest methods like jest.spyOn and jest.mock.

huh maybe we don't have any rules that actually check for those - just ones that suggest using them....

SimenB

fantastic stuff! 👏

src/rules/expect-expect.ts

src/rules/__tests__/valid-describe-callback.test.ts

src/rules/__tests__/utils.test.ts

SimenB · 2022-04-30T08:04:12Z

src/rules/__tests__/utils.test.ts

+const hooks = ['beforeAll', 'beforeEach', 'afterEach', 'afterAll'];
+
+ruleTester.run('hooks', rule, {
+  valid: [...hooks, 'beforeAll.each(() => {})'],


beforeAll.each isn't valid. But I guess the eslint plugin can just ignore it

src/rules/__tests__/utils.test.ts

src/rules/utils.ts

SimenB · 2022-04-30T08:08:22Z

src/rules/utils.ts

+}
+
+const describeImportDefAsImport = (
+  def: TSESLint.Scope.Definitions.ImportBindingDefinition,


Suggested change

def: TSESLint.Scope.Definitions.ImportBindingDefinition,

definition: TSESLint.Scope.Definitions.ImportBindingDefinition,

A little on the fence about this - I went with def because that's what the underlying runtime property that's passed in is called

SimenB · 2022-04-30T08:09:32Z

src/rules/utils.ts

+};
+
+const describeVariableDefAsImport = (
+  def: TSESLint.Scope.Definitions.VariableDefinition,


Suggested change

def: TSESLint.Scope.Definitions.VariableDefinition,

definition: TSESLint.Scope.Definitions.VariableDefinition,

SimenB · 2022-04-30T08:09:42Z

src/rules/utils.ts

+ * If it's neither of these, `null` is returned to indicate that the definition
+ * is not describable as an import of any kind.
+ */
+const describePossibleImportDef = (def: TSESLint.Scope.Definition) => {


Suggested change

const describePossibleImportDef = (def: TSESLint.Scope.Definition) => {

const describePossibleImportDef = (definition: TSESLint.Scope.Definition) => {

SimenB

great stuff 👍

…amic import

# [26.2.0](v26.1.5...v26.2.0) (2022-05-13) ### Features * support `@jest/globals` ([#1094](#1094)) ([84d7a68](84d7a68))

github-actions · 2022-05-13T21:14:00Z

🎉 This PR is included in version 26.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

G-Rath · 2022-05-14T01:24:37Z

Turns out this isn't completely right because context.getScope is actually stateful:

returns the scope of the currently-traversed node. This information can be used to track references to variables.

So while the logic is correct, it only works correctly in utils because the test rule does this:

const rule = createRule({
  // ...
  create: context => ({
    CallExpression(node) {
      const scope = context.getScope();
      // ...
    },
  }),
});

whereas in the rules we're doing:

const rule = createRule({
  // ...
  create: context => {
    const scope = context.getScope();

    return {
      CallExpression(node) {
        // ...
      },
    };
  },
});

I think for now I'm just going to replace all uses of scope with context.getScope() to be safe - I want to avoid over saturating our tests with the same extensive subset of tests, but will also chuck a few explicit ones into some of the rules just to be safe.

G-Rath · 2022-05-14T02:06:19Z

Also @SimenB I just realised we should be doing this for expect as well right?

SimenB · 2022-05-14T08:46:39Z

Yeah, all globals plus jest

G-Rath · 2022-05-14T09:14:01Z

Yeah but I'm pretty sure these + expect are the only ones we actually check for right now 😁

G-Rath added the enhancement label Apr 25, 2022

G-Rath requested a review from SimenB April 25, 2022 19:47

G-Rath removed the request for review from SimenB April 25, 2022 20:05

G-Rath force-pushed the support-jest-globals branch 3 times, most recently from 3e1ac37 to 660c04d Compare April 29, 2022 03:54

G-Rath force-pushed the support-jest-globals branch from 660c04d to 3601ceb Compare April 30, 2022 00:10

G-Rath requested a review from SimenB April 30, 2022 01:51

G-Rath marked this pull request as ready for review April 30, 2022 01:51

SimenB approved these changes Apr 30, 2022

View reviewed changes

SimenB approved these changes May 4, 2022

View reviewed changes

ecraig12345 mentioned this pull request May 10, 2022

[new-rule] prefer importing from @jest/globals #1101

Closed

G-Rath added 10 commits May 14, 2022 08:14

test: add initial cases

7cf8ba0

refactor: simplify isTestCaseCall & isDescribeCall utilities

e0f2da1

feat: check that test functions are probably from jest

00dc0a3

feat: apply to isDescribeCall too

849c1bf

fix: check import refs before unresolved refs

a0c2910

test: add utils cases

089307f

fix: begin to support variable imports

c6ee8cd

test(expect-expect): more tests

cdfb2ce

test: set correct ecmaVersion

f082eb3

chore: clean

42a7c95

G-Rath added 12 commits May 14, 2022 08:14

test: ignore coverage on condition that is only tested on eslint v8

b80a40b

ci: only collect coverage when using eslint versions that support dyn…

e118101

…amic import

feat: convert isHook to isHookCall

23f5365

chore: remove unneeded exporting of util function

6791de6

refactor(expect-expect): reuse scope

4fc6a57

refactor: compare against null instead of casting to boolean

f85b0d9

chore: mark array param as readonly

68b9760

test: move utils case from specific rule tests into utils tests

556edc3

refactor: rename variable

f13ba6b

test: add some more cases

c0ef8e5

chore: remove duplicated test case

8a06778

test: let original error throw if eslint cannot be found

8660f44

G-Rath force-pushed the support-jest-globals branch from e09c7eb to 8660f44 Compare May 13, 2022 20:49

chore: upgrade @typescript-eslint/types

c832563

G-Rath changed the title ~~feat: support jest globals~~ feat: support @jest/globals May 13, 2022

G-Rath merged commit 84d7a68 into main May 13, 2022

G-Rath deleted the support-jest-globals branch May 13, 2022 21:03

G-Rath mentioned this pull request May 13, 2022

Support resolving underlying @jest/globals method from import #1106

Closed

github-actions bot pushed a commit that referenced this pull request May 13, 2022

chore(release): 26.2.0 [skip ci]

7dd707f

# [26.2.0](v26.1.5...v26.2.0) (2022-05-13) ### Features * support `@jest/globals` ([#1094](#1094)) ([84d7a68](84d7a68))

github-actions bot added the released label May 13, 2022

G-Rath mentioned this pull request May 14, 2022

fix: use correct scope for checking references #1107

Merged

roschaefer mentioned this pull request May 14, 2022

Error: Reference unexpected had more than one definition - please file a github issue #1108

Closed

ybiquitous mentioned this pull request May 16, 2022

Bump eslint-plugin-jest from 26.1.5 to 26.2.2 stylelint/stylelint#6096

Merged

G-Rath mentioned this pull request May 28, 2022

feat: improve how jest function calls are resolved to account for import aliases #1122

Merged

G-Rath mentioned this pull request Jul 1, 2022

feat: bunch of updates jest-community/eslint-plugin-jest-extended#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support `@jest/globals` #1094

feat: support `@jest/globals` #1094

G-Rath commented Apr 25, 2022 •

edited

SimenB commented Apr 25, 2022

G-Rath commented Apr 25, 2022

SimenB commented Apr 25, 2022

SimenB commented Apr 25, 2022 •

edited

G-Rath commented Apr 28, 2022

SimenB commented Apr 29, 2022

G-Rath commented Apr 30, 2022 •

edited

G-Rath commented Apr 30, 2022

SimenB left a comment

SimenB Apr 30, 2022

SimenB Apr 30, 2022

G-Rath May 1, 2022

SimenB Apr 30, 2022

SimenB Apr 30, 2022

SimenB left a comment

github-actions bot commented May 13, 2022

G-Rath commented May 14, 2022

G-Rath commented May 14, 2022

SimenB commented May 14, 2022

G-Rath commented May 14, 2022

	def: TSESLint.Scope.Definitions.ImportBindingDefinition,
	definition: TSESLint.Scope.Definitions.ImportBindingDefinition,

	def: TSESLint.Scope.Definitions.VariableDefinition,
	definition: TSESLint.Scope.Definitions.VariableDefinition,

	const describePossibleImportDef = (def: TSESLint.Scope.Definition) => {
	const describePossibleImportDef = (definition: TSESLint.Scope.Definition) => {

feat: support @jest/globals #1094

feat: support @jest/globals #1094

Conversation

G-Rath commented Apr 25, 2022 • edited

SimenB commented Apr 25, 2022

G-Rath commented Apr 25, 2022

SimenB commented Apr 25, 2022

SimenB commented Apr 25, 2022 • edited

G-Rath commented Apr 28, 2022

SimenB commented Apr 29, 2022

G-Rath commented Apr 30, 2022 • edited

G-Rath commented Apr 30, 2022

SimenB left a comment

Choose a reason for hiding this comment

SimenB Apr 30, 2022

Choose a reason for hiding this comment

SimenB Apr 30, 2022

Choose a reason for hiding this comment

G-Rath May 1, 2022

Choose a reason for hiding this comment

SimenB Apr 30, 2022

Choose a reason for hiding this comment

SimenB Apr 30, 2022

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2022

G-Rath commented May 14, 2022

G-Rath commented May 14, 2022

SimenB commented May 14, 2022

G-Rath commented May 14, 2022

feat: support `@jest/globals` #1094

feat: support `@jest/globals` #1094

G-Rath commented Apr 25, 2022 •

edited

SimenB commented Apr 25, 2022 •

edited

G-Rath commented Apr 30, 2022 •

edited