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.

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

fix(takeLast): fix takeLast behavior to emit correct order #1412

Merged
merged 1 commit into from
Mar 3, 2016

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Mar 1, 2016

closes #1407

This PR tries to fix behavior of takeLast by changing ring buffer indexing routines. Maybe I miss something, but so far it looks work correctly.

/cc @staltz for visibility.

@kwonoj
Copy link
Member Author

kwonoj commented Mar 2, 2016

Found one failure cases, marking as blocked for now. updated PR includes additional testcase.

@staltz
Copy link
Member

staltz commented Mar 2, 2016

Nice! Thanks for the fix

expect(expected.length).toBe(0);
done();
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Can we change the above tests to use marble diagrams and virtual scheduling?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, let me do that.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind keeping one synchronous test, of course, but the marble tests are more analogous to the issue that was reported (which was using an interval).

@kwonoj
Copy link
Member Author

kwonoj commented Mar 2, 2016

Updated PR to use marble diagram for test cases.

it('should take last three values', () => {
const e1 = cold('--a-----b----c---d--| ');
const e1subs = '^ ! ';
const expected = '--------------------(bcd|)';
Copy link
Member

Choose a reason for hiding this comment

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

Much nicer

@kwonoj kwonoj merged commit 73eb658 into ReactiveX:master Mar 3, 2016
@kwonoj
Copy link
Member Author

kwonoj commented Mar 3, 2016

Merged with 73eb658.

@kwonoj kwonoj deleted the fix-takelast branch March 3, 2016 07:02
@lock
Copy link

lock bot commented Jun 7, 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 7, 2018
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.

takeLast bug
3 participants