Skip to content

Conversation

Chaoba
Copy link

@Chaoba Chaoba commented Dec 30, 2015

This PR complete the never assert in PublishSubjectTest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll want verifyNoMoreInteractions(observer) to end these two methods, otherwise sending an onNext of "five" would not fail the test.

@akarnokd
Copy link
Member

👍

@vanniktech
Copy link
Collaborator

In every test method that is using verifyNoMoreInteractions you can remove the verify(observer, never()).*

@Chaoba
Copy link
Author

Chaoba commented Dec 31, 2015

In every test method that is using verifyNoMoreInteractions you can remove the verify(observer, never()).*

Your are right, but I think adding these verifies can make tests more clearly.

@vanniktech
Copy link
Collaborator

Fair enough. I just thought you forgot to delete them and wanted to point it out.
In any case the deletion decision is something the repo owners have to do.

Personally the less test code the preciser it is. Especially since verifyNoMoreInteractions clearly states it purpose.

@akarnokd akarnokd changed the title 1.x: add never test for PublishSuibjectTest 1.x: add never test for PublishSubjectTest Dec 31, 2015
@zsxwing
Copy link
Member

zsxwing commented Jan 11, 2016

👍

akarnokd added a commit that referenced this pull request Jan 11, 2016
1.x: add never test for  PublishSubjectTest
@akarnokd akarnokd merged commit 52a7e49 into ReactiveX:1.x Jan 11, 2016
@stevegury
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants