Skip to content

1.x: add never test for PublishSubjectTest #3592

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

Merged
merged 2 commits into from
Jan 11, 2016
Merged

1.x: add never test for PublishSubjectTest #3592

merged 2 commits into from
Jan 11, 2016

Conversation

Chaoba
Copy link

@Chaoba Chaoba commented Dec 30, 2015

This PR complete the never assert in PublishSubjectTest.

verify(observer, never()).onNext("four");
verify(observer, times(1)).onError(any(Throwable.class));
verify(observer, never()).onCompleted();
}
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