search

re-rxjs / react-rxjs

React bindings for RxJS
https://react-rxjs.org
MIT License
549 stars 19 forks source link

fix(utils-split): dismiss inner observable on error #112

Closed josepot closed 4 years ago

josepot commented 4 years ago

ups!

codecov[bot] commented 4 years ago

Codecov Report

Merging #112 into main will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #112   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          363       363           
  Branches        35        35           
=========================================
  Hits           363       363
Impacted Files Coverage Δ
packages/utils/src/split.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0515a11...d0eba02. Read the comment docs.

josepot commented 4 years ago

Is this use case covered by a test?

Of course, our code-coverage is 100%! that means that everything is covered by tests, right?

Just kidding! I've been trying to write a test for this, but I'm having a very hard time coming up with a decent one, but you are right, I won't merge it until I can write that test. Just to be clear, the test should not pass on the current main branch and it should pass after these changes.

Thanks @rikoe !

josepot commented 4 years ago

@all-contributors please add @rikoe for review

allcontributors[bot] commented 4 years ago

@josepot

I've put up a pull request to add @rikoe! :tada:

josepot commented 4 years ago

The first commit is a test that doesn't pass on main (I made sure that the CI ran on that commit and that the test fails) and the second commit is the one that fixes the issue.