fix(ErrorObservable): Added and fixed failing test for Observable.catch #2552
Conversation
trshafer
force-pushed
the
observable-throw-from-promise
branch
3 times, most recently
from
49dae47
to
a5e9c33
Compare
trshafer
changed the title
trshafer
force-pushed
the
observable-throw-from-promise
branch
from
f9a64c6
to
4e2a978
Compare
spec/operators/catch-spec.ts
Outdated
| let timers: sinon.SinonFakeTimers; | ||
|
|
||
| let source: Rx.Observable<any>; | ||
| beforeEach(() => { |
There was a problem hiding this comment.
These tests are a little too DRY for my liking. Prefer doing as much set up as possible in each test.
Also, I'll be honest, I'm not at all familar with sinon.sandbox. I'll need to research it to see if it's an approach we want to use. I presume it's some virtualized scheduling thing.
There was a problem hiding this comment.
Hey @benlesh, thanks for taking a look at this PR. I'll update the tests to be more explicit per test and get back to you. I used sinon.sandbox as I found other examples in the current test suite: https://github.com/ReactiveX/rxjs/search?utf8=%E2%9C%93&q=sinon.sandbox&type=
There was a problem hiding this comment.
we have few cases of using it, but in general if it can be avoided try not to use it. except some obvious cases (like test scheduler), usually test for Rx can be written without sandbox.
There was a problem hiding this comment.
I removed the source observable from the setup block and into each test.
trshafer
force-pushed
the
observable-throw-from-promise
branch
from
4e2a978
to
0f4bb92
Compare
| @@ -278,4 +279,83 @@ describe('Observable.prototype.catch', () => { | |||
| done(); | |||
| }); | |||
| }); | |||
|
|
|||
| context('fromPromise', () => { | |||
There was a problem hiding this comment.
I know this is a nitpick... but everywhere else I think we use describe. I didn't even know context is a thing. Is there a reason this is better in this case than describe?
There was a problem hiding this comment.
Happy to change it to describe, didn't realize there are no other usages of context in the test suite. I personally have used context to mean "a different approach to call the same feature" where describe is a "different feature". There is no difference in functionality.
describe('functionA', () => {
context('success', () => {...});
context('error', () => {...});
});Documentation: https://mochajs.org/#bdd
|
Thanks, @trshafer! |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description:
Added a failing test describing unexpected behavior between throw Error and Observable.throw(Error) when the source of an observable is a Promise. In my opinion this behavior is confusing as the catch handler needs to know if the source is from a Promise vs another source. Thanks for taking a look.
Added fix by forcing
sync.syncErrorThrowableonErrorObservable. Let me know if this is a good fix and if I should add a unit test for ErrorObservable.Related issue (if exists):
Related #2485.