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

rfc: supertest support #418

Closed
G-Rath opened this issue Sep 29, 2019 · 5 comments
Closed

rfc: supertest support #418

G-Rath opened this issue Sep 29, 2019 · 5 comments
Labels

Comments

@G-Rath
Copy link
Collaborator

G-Rath commented Sep 29, 2019

This could be technically out of scope for eslint-plugin-jest, but I've been using supertest, and some of our rules don't really work nicely w/ it:

test('GET /live', async () => {
  await request(app)
    .get('/live')
    .expect('Content-Type', /json/)
    .expect(HttpStatus.OK)
    .expect('');
});

jest/expect-expect can't handle .expect as a method being called on something, so I have to add request, which means I could forget a .expect and have problems.

It'd also be great to support warning that request needs to be either awaited or returned, as it's a promise. I think this could be covered by one of our existing rules.

Using return also annoys jest/no-test-return-statement.

I don't think we could catch every possible way of using supertest, and I've only just started using it myself, but I'm interested in gathering some initial comments & thoughts on if there's any low-hanging fruit we could aim for that'd improve things w/o much work.

The main one for me is improving expect-expect to support .expect() - logic-wise easy, but the configuration would be interesting.

Theres also an argument that could be made about jest/lowercase-name supporting ignoring GET /live.

Right now, I'm feeling like this might be worth it's own plugin at some point, but as far as I know theres no nice way of composing rules - instead you'd just have to clone rules like jest/lowercase-name into eslint-plugin-supertest, a la @typescript-eslint does w/ the eslint rules.

@jeysal
Copy link
Member

jeysal commented Sep 30, 2019

Seems like this also falls into a more general category of "asserting" that a promise resolves (does not reject or timeout), it could even be legitimate without any .expects right?

It'd also be great to support warning that request needs to be either awaited or returned, as it's a promise. I think this could be covered by one of our existing rules.

I think we should leave this for no-floating-promises to do. Of course for non-TS users we should still keep the existing rule regarding Jest's built-in resolves/rejects.

@SimenB
Copy link
Member

SimenB commented Sep 30, 2019

I don't think this belongs this plugin. Seems more natural in a eslint-plugin-supertest or whatever.

We use supertest at work, but we never use the assertions from it. Mostly because they don't give proper stack traces.

test('GET /live', async () => {
  const { headers, status, body } = await request(app).get('/live');

  expect(headers.contentType).toMatch(/json/);
  expect(status).toBe(200);
  expect(body).toBe('');
});

(written free hand, haven't looked up the actual APIs)

@G-Rath
Copy link
Collaborator Author

G-Rath commented Nov 8, 2019

I'm going to close this b/c I think it served its purpose; worse case I can always pop further questions here anyway.

@G-Rath G-Rath closed this as completed Nov 8, 2019
@erunion
Copy link
Contributor

erunion commented Nov 9, 2019

We stumbled upon this with Sinon assertions not being picked up.

const spy = sinon.spy()
it('should work as expected', async () => {
  // test code here

  sinon.assert.calledOnce(spy);
});

Adding sinon (or calledOnce) into the assertFunctionNames configuration block doesn't get recognized, so it would be cool to be able to add glob-style matching into it. Something like sinon.assert.* or request.*.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Nov 9, 2019

Thanks for that @erunion - would you mind opening that as a separate issue for tracking?

At least you should be able to put something like sinon.assert.calledOnce into assertFunctionNames and have it work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants