fix(valid-expect): validate async expect calls #78

macklinu · 2018-02-13T18:31:51Z

Resolves #54

I first tried working my way up the tree - to find a parent node of an expect() call that was a ReturnStatement or AwaitStatement and then finding the function that contained the expect() call, but I had trouble thinking through how to do that with the current valid-expect rule code. I ended up checking for a test case CallExpression and working my way down the tree - into the body of the callback function:

test("foo", () => {
  // validate each `expect().resolves` and `expect().rejects` in this block
})

One downside is that this results in some really deeply nested property accessing (like node.expression.callee.object.object.arguments[0].type).

Am I approaching this the right way? Feedback is certainly welcome on how to improve this.

macklinu

It would also be a good idea for me to update the valid-expect documentation to explain these cases. 👍

macklinu · 2018-02-13T18:32:40Z

rules/valid-expect.js

+    return false;
+  }
+  const callee = node.expression.callee;
+  return (


This could be improved with something like _.get or get-value.

I'm down with that. Will make coverage a bit easier as well 🙂

Do you have preferences over which dependency to add? get-value is smaller, although I don't know if that's as big a deal with ESLint plugins (for example, I would be more concerned about adding the entire Lodash dependency for a web app than a command line tool).

no preference 🙂

macklinu · 2018-02-13T18:33:51Z

rules/__tests__/valid-expect.test.js

+    },
+    {
+      code:
+        'test("foo", async () => expect(Promise.reject(2)).rejects.toBeDefined());',


This is technically valid, since this arrow function implicitly returns the expect() statement. However, the test() callback function is async, so I figured we could warn when you're using async without await in this case.

Thoughts on this?

I think that should be fine as a separate rule.

https://github.com/avajs/eslint-plugin-ava/blob/master/docs/rules/no-async-fn-without-await.md

Ok, I can remove that check from this rule then, and we can open an issue for another rule.

Created #81 so this wouldn't get lost

macklinu · 2018-02-14T14:40:33Z

rules/__tests__/valid-expect.test.js

+    'test("foo", () => expect(Promise.resolve(2)).resolves.toBeDefined());',
+    'test("foo", async () => await expect(Promise.reject(2)).rejects.toBeDefined());',
+    'test("foo", async () => { expect(await Promise.resolve(2)).resolves.toBeDefined(); });',
+    'test("foo", async () => { expect(await Promise.reject(2)).rejects.toBeDefined(); });',


Based on the comments in #54 (comment), I might remove the logic surrounding these test cases, as it's not valid to await the Promise inside expect() and also use resolves or rejects.

It'll work, it just doesn't make any sense. So I think we should warn in that case as you're doing currently

Actually, I was wrong, jest'll throw.

so if anything it should be added to invalid 🙂

SimenB · 2018-02-16T07:11:09Z

conflicts again, sorry

macklinu · 2018-02-16T14:43:15Z

Thank you for the review, @SimenB - I'll fix the conflicts and make the necessary changes today.

macklinu

Updated the test cases and documentation. This is now ready for review.

Please let me know if I should refactor anything for naming or extract any functions (specifically the filter() callback functions). I inlined everything while I was addressing PR feedback to help me better see the flow of the code I wrote 😂

macklinu · 2018-02-16T15:30:15Z

rules/valid-expect.js

    return {
      CallExpression(node) {
+        if (util.isTestCase(node)) {
+          validateAsyncExpects(node);
+          return;


If we are in a test case function, there is no need to proceed, since the node.callee.name will never be expect.

SimenB

This is awesome, thank you so much for tackling it!

SimenB · 2018-02-17T12:05:34Z

rules/valid-expect.js

+            return (
+              node.expression.type === 'CallExpression' &&
+              objectName === 'expect' &&
+              (propertyName === 'resolves' || propertyName === 'rejects')


I wonder if we can reuse the logic somehow with the other resolves/rejects checks

Good idea. I'll look into that while resolving the failing test case you reported below.

SimenB · 2018-02-17T12:06:19Z

rules/__tests__/valid-expect.test.js

+    {
+      code:
+        'test("foo", () => { expect(Promise.resolve(2)).resolves.toBeDefined(); });',
+      errors: [{ message: "Must return or await 'expect.resolves' statement" }],


can you assert on the locations as well?

Yep, will make those changes 👍

SimenB

Actually, this should be expanded to support .not.

This test fails:

    {
      code:
        'test("foo", async () => { expect(await Promise.reject(undefined)).rejects.not.toBeDefined(); });',
      errors: [
        { message: "Cannot use 'rejects' with an awaited expect expression" },
      ],
    },

SimenB · 2018-02-25T22:00:40Z

I don't get emails when people push to PRs I'm reviewing for some reason 🙁 So please ping me when this is ready

macklinu · 2018-02-25T22:15:16Z

Will do. I need to sit down this week and give this PR a solid look to see how we can share logic between the current behavior and the new behavior I’m introducing.

…

On Sun, Feb 25, 2018 at 5:00 PM Simen Bekkhus ***@***.***> wrote: I don't get emails when people push to PRs I'm reviewing for some reason 🙁 So please ping me when this is ready — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#78 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPEyZf4h27F9AZVXDmSdYRYrn5_e_pxks5tYdgIgaJpZM4SEM-b> .

macklinu · 2018-02-28T23:23:40Z

@SimenB I'm in-between jobs and moving right now and probably won't be available to contribute again until late March/early April. Thoughts on what to do with this PR? Just leave it open for now?

SimenB · 2018-03-01T07:39:36Z

@macklinu there is no rush, I think it's fine to leave it open.

macklinu · 2018-03-09T15:54:04Z

I may be able to re-approach this PR by using context.getAncestors() instead of traversing the nodes within the callback of a test() function. I think this could help us reuse the current logic for checking the usage of .not, .resolves, and .rejects.

Here is a simplified code example to show how to check if an expect() CallExpression was returned:

const rule = {
  CallExpression(node) {
    if (
      node.callee.name === "expect" &&
      context
        .getAncestors()
        .some(
          n =>
            n.type === "ReturnStatement" &&
            n.argument.callee.object.object === node
        )
    ) {
      // expect() was returned
    }
  }
};

I think I'll be able to hack on this later today / this weekend - will see where I end up. 😄

Resolves jest-community#54

macklinu · 2018-05-31T20:07:56Z

I'm not sure this is the right approach, and I haven't been able to take a look for quite some time. Closing for now.

macklinu commented Feb 13, 2018

View reviewed changes

macklinu commented Feb 14, 2018

View reviewed changes

macklinu force-pushed the fix/valid-expect-async branch from 2b1f60c to 4fa5083 Compare February 15, 2018 19:11

macklinu force-pushed the fix/valid-expect-async branch from 4fa5083 to 49f3b1b Compare February 16, 2018 15:28

macklinu commented Feb 16, 2018

View reviewed changes

macklinu mentioned this pull request Feb 16, 2018

New rule: no-async-without-await #81

Closed

SimenB approved these changes Feb 17, 2018

View reviewed changes

SimenB reviewed Feb 17, 2018

View reviewed changes

SimenB requested changes Feb 17, 2018

View reviewed changes

macklinu force-pushed the fix/valid-expect-async branch from 49f3b1b to deb8016 Compare February 24, 2018 21:14

macklinu added 2 commits March 9, 2018 14:46

fix(valid-expect): validate async expect calls

091d787

Resolves jest-community#54

test: add failing tests from PR comment

d096af7

macklinu force-pushed the fix/valid-expect-async branch from deb8016 to d096af7 Compare March 9, 2018 19:46

macklinu mentioned this pull request Mar 13, 2018

chore: add parseExpect() util function #101

Closed

macklinu closed this May 31, 2018

macklinu mentioned this pull request May 31, 2018

Assert that async expects are awaited or returned #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(valid-expect): validate async expect calls #78

fix(valid-expect): validate async expect calls #78

macklinu commented Feb 13, 2018

macklinu left a comment

macklinu Feb 13, 2018

SimenB Feb 14, 2018

macklinu Feb 14, 2018

SimenB Feb 16, 2018

macklinu Feb 13, 2018

SimenB Feb 14, 2018

macklinu Feb 14, 2018

macklinu Feb 16, 2018

macklinu Feb 14, 2018

SimenB Feb 14, 2018

SimenB Feb 16, 2018

SimenB Feb 16, 2018

SimenB commented Feb 16, 2018

macklinu commented Feb 16, 2018

macklinu left a comment

macklinu Feb 16, 2018

SimenB left a comment

SimenB Feb 17, 2018

macklinu Feb 17, 2018

SimenB Feb 17, 2018

macklinu Feb 17, 2018

SimenB left a comment

SimenB commented Feb 25, 2018

macklinu commented Feb 25, 2018 via email

macklinu commented Feb 28, 2018

SimenB commented Mar 1, 2018

macklinu commented Mar 9, 2018

macklinu commented May 31, 2018

fix(valid-expect): validate async expect calls #78

fix(valid-expect): validate async expect calls #78

Conversation

macklinu commented Feb 13, 2018

macklinu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Feb 16, 2018

macklinu commented Feb 16, 2018

macklinu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Feb 25, 2018

macklinu commented Feb 25, 2018 via email

macklinu commented Feb 28, 2018

SimenB commented Mar 1, 2018

macklinu commented Mar 9, 2018

macklinu commented May 31, 2018