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.

Sign up for GitHub

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

Jump to bottom

Typo/error in withLatestFrom() test? #2526

Closed
apaatsio opened this issue Apr 5, 2017 · 4 comments · Fixed by #2578
Closed

Typo/error in withLatestFrom() test? #2526

apaatsio opened this issue Apr 5, 2017 · 4 comments · Fixed by #2578
Labels
bug Confirmed bug help wanted Issues we wouldn't mind assistance with.

Comments

@apaatsio
Copy link
Contributor

apaatsio commented Apr 5, 2017

There is a test called "should combine events from cold observables" but the test itself uses hot().

Link to the section in code: https://github.com/ReactiveX/rxjs/blob/master/spec/operators/withLatestFrom-spec.ts#L16-L24

Here's the test code:

  asDiagram('withLatestFrom')('should combine events from cold observables', () => {
    const e1 =   hot('-a--b-----c-d-e-|');
    const e2 =   hot('--1--2-3-4---|   ');
    const expected = '----B-----C-D-E-|';

    const result = e1.withLatestFrom(e2, (a: string, b: string) => String(a) + String(b));

    expectObservable(result).toBe(expected, { B: 'b1', C: 'c4', D: 'd4', E: 'e4' });
  });
@kwonoj
Copy link
Member

kwonoj commented Apr 16, 2017

uh..seems so? I can't recall which one's intended though (description vs. test) 😅

@kwonoj kwonoj added bug Confirmed bug help wanted Issues we wouldn't mind assistance with. labels Apr 16, 2017
@kwonoj
Copy link
Member

kwonoj commented Apr 16, 2017

marking as 'bug', at least it's incorrect state.

@martinsik
Copy link
Contributor

martinsik commented Apr 25, 2017 •
edited

I think it should use cold() because hot Observables are tested right in the next test case: https://github.com/ReactiveX/rxjs/blob/master/spec/operators/withLatestFrom-spec.ts#L26

@Boshen Boshen mentioned this issue Apr 25, 2017
Merged
@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Confirmed bug help wanted Issues we wouldn't mind assistance with.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(withLatestFrom): change from hot to cold observable in marble test
3 participants
@apaatsio @martinsik @kwonoj