search

evilsoft / crocks

A collection of well known Algebraic Data Types for your utter enjoyment.
https://crocks.dev
ISC License
1.59k stars 102 forks source link

Fix unhandled promise rejection in tests #482

Closed benhormann closed 4 years ago

benhormann commented 4 years ago

Found an error message when running the tests.

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 2bad6aaa6078465a80745b2d85eebfa39a06ad27 on benhormann:unhandled-promise-rejection into dd0b5307414c3709a7951573ea4fab95b59852d0 on evilsoft:master.

dalefrancis88 commented 4 years ago

Thanks for contributing, sorry this slipped under my radar and it's been 10 days.

I'd be curious to understand "what gives" if the promise remains un-handled? Or said differently, what led you to seeing this issue and what insight/value do we gain from handling it?

This is more to be informed in the future, on the base of this i don't see a reason not to approve

benhormann commented 4 years ago

Node's default is to emit Event: 'unhandledRejection' and a deprecation warning. The warning mentions In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.; You can turn on this behaviour with --unhandled-rejections=strict.

It really only matters if the default or CI / test runner settings change.

I discovered it in the output of npm run test, it is hidden when using spec:dev.

benhormann commented 4 years ago

On second thought, it might look better like this?


  const rejected = Promise.reject(0)
  rejected.catch(() => 'Handle promise rejection, avoid error.')

  t.equal(isPromise(Promise.resolve(0)), true, 'returns true when passed a resolved promise')
  t.equal(isPromise(rejected), true, 'returns true when passed a rejected promise')
dalefrancis88 commented 4 years ago

Thanks for the extra context. I am a big fan of the arrange, act, assert structure so i like the second option. That being said, both handle the raised issue. I'm happy to approve as is or your welcome to change. I'll leave it to you

dalefrancis88 commented 4 years ago

@evilsoft it appears travis doesn't want to play ball again, Is it because of a force-push?

benhormann commented 4 years ago

Would rebasing onto the previous commit help?

If this project squashes PR commits by default, may I suggest a note about it (and avoiding force pushes) in CONTRIBUTING?

evilsoft commented 4 years ago

Oh snap, so sorry I missed this @dalefrancis88 Looks like it just did not update, But this is all fixed up and 🔴 👁️ to merge when you want.

@benhormann GREAT catch on this btw, thanks you so much for your time and effort!