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: https://github.com/ReactiveX/RxGo/issues/234 #235

Closed
wants to merge 1 commit into from

Conversation

v-zubko
Copy link
Contributor

@v-zubko v-zubko commented Feb 26, 2020

Maybe Window function also should be addressed?

@coveralls
Copy link

coveralls commented Feb 26, 2020

Pull Request Test Coverage Report for Build 764

  • 85 of 114 (74.56%) changed or added relevant lines in 1 file are covered.
  • 14 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.07%) to 84.199%

Changes Missing Coverage Covered Lines Changed/Added Lines %
observable_operator.go 85 114 74.56%
Files with Coverage Reduction New Missed Lines %
observable.go 1 86.47%
observable_operator.go 13 81.6%
Totals Coverage Status
Change from base Build 758: 0.07%
Covered Lines: 2787
Relevant Lines: 3310

💛 - Coveralls

@teivah teivah self-requested a review February 27, 2020 09:09
Copy link
Member

@teivah teivah left a comment

Choose a reason for hiding this comment

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

Thanks @v-zubko but the tests are failing.

@v-zubko
Copy link
Contributor Author

v-zubko commented Feb 27, 2020

Yup, I saw :) @teivah Does this change go in a right way? Or you would do it different? Do I have to update BufferWithTimeOrCount, WindowWithTime and WindowWithTimeOrCount?

@teivah
Copy link
Member

teivah commented Feb 27, 2020

Yup, I saw :) @teivah Does this change go in a right way? Or you would do it different? Do I have to update BufferWithTimeOrCount, WindowWithTime and WindowWithTimeOrCount?

Yes, I like the approach. Let's forget about this causalityDuration for the time being and keep things simple.
If you want to tackle the others as well, yes please :)

@v-zubko v-zubko force-pushed the BufferWithTime-bug branch 7 times, most recently from 0953cd4 to c6974ba Compare February 27, 2020 16:36
@teivah
Copy link
Member

teivah commented Feb 27, 2020

@v-zubko Some tests are failing like Test_Observable_WindowWithTime

@v-zubko
Copy link
Contributor Author

v-zubko commented Feb 27, 2020

Where are they failing? CI is green :)

@teivah
Copy link
Member

teivah commented Feb 27, 2020

On my local env:

        	Error Trace:	assert.go:225
        	            				observable_operator_test.go:1665
        	Error:      	Not equal: 
        	            	expected: []interface {}{5, 6, 7}
        	            	actual  : []interface {}{5, 6, 7, 8}

Try repeating them. Also, some tests like Test_Observable_BufferWithTime are taking 9 seconds.

@v-zubko
Copy link
Contributor Author

v-zubko commented Feb 27, 2020

9 seconds is expected. Cause of delays.

@v-zubko
Copy link
Contributor Author

v-zubko commented Feb 27, 2020

BTW are you sure that you've got latest version? Cause I see another expected values:

Assert(ctx, t, JustItem((<-observe).V), HasItems(1, 2))
Assert(ctx, t, JustItem((<-observe).V), HasItems(3, 4, 5))
Assert(ctx, t, JustItem((<-observe).V), HasItems(6, 7, 8))

@teivah
Copy link
Member

teivah commented Feb 28, 2020

BTW are you sure that you've got latest version? Cause I see another expected values:

Assert(ctx, t, JustItem((<-observe).V), HasItems(1, 2))
Assert(ctx, t, JustItem((<-observe).V), HasItems(3, 4, 5))
Assert(ctx, t, JustItem((<-observe).V), HasItems(6, 7, 8))

Nope, check at the files changed on this PR.

It's still:

Assert(ctx, t, (<-observe).V.(Observable), HasItems(0, 1))
Assert(ctx, t, (<-observe).V.(Observable), HasItems(2, 3, 4))
Assert(ctx, t, (<-observe).V.(Observable), HasItems(5, 6, 7))

Moreover, we can't keep a unit test to executes in 9 seconds.

@teivah
Copy link
Member

teivah commented Mar 22, 2020

No longer active

@teivah teivah closed this Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants