fix-405

[G-Rath]

I'm not entirely sure why this is needed, as it doesn't seem to be different from espree.

Still, all the tests are now passing, so I call it a win.

Fixes #405

[SimenB]

This is not the correct fix - we just don't have any tests for async functions.

E.g. this works correctly if added to invalid tests to master, but fails on this branch

{
  code: `
    it('test function', async () => {
      Builder.getPromiseBuilder().get().build().then((data) => expect(data).toEqual('Hi'));
    });
  `,
  errors: [{ column: 11, endColumn: 96, messageId: 'returnPromise' }],
},

It's just a copy of another invalid test, but with added async keyword

[G-Rath]

😬 my apologies - for some reason I thought we had async function tests.

I've implemented the proper solution :)

[SimenB]

Could we verify all non-async test cases have an async version and vice versa? Also add in the example from the issue as valid

[G-Rath]

More than happy to tomorrow morning (it's 12am here) - also happy if you want to push to this branch :)

Also add in the example from the issue as valid

That has already been added in :) my rebasing skipped it 😂

[SimenB]

will do. thanks!

[SimenB]

not covered

[SimenB]

not covered

[G-Rath]

@SimenB The uncovered lines are not actually coverable w/o some more refactoring.

We can "cover" the second return false by just removing it, but the first one will have to be ignored.

Having re-read the material, I think the problem here is that this rule is actually meant to be about expects inside promises; so await isn't something it should care about - that's instead handled by valid-expect.

The problem in this case is that we bail out if the parent is an await expression, which means the parents parent isn't checked :/

[G-Rath]

@SimenB Have you had a chance to look over this yet? (just in case the email got lost again 😂)

[SimenB]

I haven't - super crunch at work for the last month...

The problem in this case is that we bail out if the parent is an await expression, which means the parents parent isn't checked :/

Can we recurse until we hit the test?

(I haven't re-read this or the issue, so context is missing atm)

[SimenB]

what's the current status here?

[G-Rath]

😬

I believe the status is that I think we're got a rule that's overreaching and so coming up a bit short:

Having re-read the material, I think the problem here is that this rule is actually meant to be about expects inside promises; so await isn't something it should care about - that's instead handled by valid-expect.

Looking at the tests & the issue, I think it should be that valid-expect-in-promise should only care if an expect is actually in a promise.

I'm a little bit of two minds on this, but I think that's because it's a bit of a confusing one to follow what with valid-expect vs valid-expect-in-promise - my reasoning is that if you take the example in the issue:

test('valid-expect-in-promise', async () => {
    const text = await fetch('url')
        .then(res => res.text()) 
        .then(text => text);     

    expect(text).toBe('text');
});

If you remove the await that's technically wrong but none of our rules will catch, but jest will because the test itself will fail; and that's where I think we should leave it, because otherwise valid-expect-in-promise would have to somehow account for these:

test('valid-expect-in-promise', async () => {
    const text = fetch('url')
        .then(res => res.text()) 
        .then(text => text);     

    expect(text).toBe('text');
    expect(await text).toBe('text');
    expect(text).resolves.toBe('text');
    expect(text).toBe(Promise.resolve); // or something...?
    expect(text).toBeInstanceOf(MyCustomPromiseClass);
});

Currently valid-expect-in-promise will error on all of these, when they're actually valid depending on what you're testing.

[G-Rath]

@SimenB have you had a chance to look over my last comment?

[github-actions]

🎉 This issue has been resolved in version 24.6.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 25.0.0-next.6 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀