Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Tidy DataQueryEndpoints tests #239

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

timoxley
Copy link
Contributor

@timoxley timoxley commented Apr 19, 2021

Fixes distracting and unnecessary TypeError issues reported in tests.

image

Also converts the supertest tests to use async/await instead of Jest's done callback.

Shouldn't conflict with #238

@timoxley timoxley requested a review from harbu April 19, 2021 20:51
@timoxley timoxley changed the title Tidy DataQueryEndpoints test Tidy DataQueryEndpoints tests Apr 19, 2021
@timoxley timoxley requested a review from teogeb April 19, 2021 20:52
})
stream.on('error', (err) => {
try {
let delimiter = ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If stream.on is failing, it seems like a fundamental problem with the code that should cause a crash instead of being wrapped in a 500 error code and sent back to client? I.e. it is an unexpected error that should not really ever occur unless there is a problem with our code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's true

it('responds 500 and error message if networkNode signals error', (done) => {
networkNode.requestResendRange = () => intoStream.object(Promise.reject(new Error('error')))
it('responds 500 and error message if networkNode signals error', async () => {
networkNode.requestResendRange = jest.fn(() => intoStream.object(Promise.reject(new Error('expected error'))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning behind wrapping the function with jest.fn if it not being asserted (e.g. num of calls)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just consistency really, jest.fn also signals (to me at least) that this is a mock function.

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

Successfully merging this pull request may close these issues.

None yet

3 participants