/RxJava

2.x: test sync + cleanup #4204

Merged
merged 6 commits into from Jul 18, 2016
+3,291 −612

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement Test
Projects
None yet
Milestone
  2.0 RC 1
3 participants
@akarnokd @codecov-io @artem-zinnatullin
@akarnokd
Member

akarnokd commented Jul 14, 2016

  • More unit tests ported;
  • TestObserver cleanup and sync with TestSubscriber;
  • fix travis to run with Java 7 instead of 8;
  • added AnimalSniffer;
  • cleaned up combineLatest, introduced combineLatestDelayError;
  • test names are as in 1.x, please don't complain about the test prefix in the method names!

@akarnokd akarnokd added Enhancement Test labels Jul 14, 2016

akarnokd added this to the 2.0 RC 1 milestone Jul 14, 2016

@codecov-io

codecov-io commented Jul 14, 2016 • edited

Current coverage is 68.54%

Merging #4204 into 2.x will increase coverage by 0.32%

@@                2.x      #4204   diff @@
==========================================
  Files           409        411     +2   
  Lines         29450      29486    +36   
  Methods           0          0          
  Messages          0          0          
  Branches       4763       4777    +14   
==========================================
+ Hits          20092      20211   +119   
+ Misses         7331       7262    -69   
+ Partials       2027       2013    -14

Powered by Codecov. Last updated by 0705001...0be4534
@akarnokd
Member

akarnokd commented Jul 17, 2016

Anybody wants to review this?
@artem-zinnatullin
Contributor

artem-zinnatullin commented Jul 18, 2016

Yes, will do in couple days (busy, sorry)

On Mon, 18 Jul 2016, 01:22 David Karnok, notifications@github.com wrote:

Anybody wants to review this?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA7B3AUegl86uvFWdaJQYs7m9Jo1hayRks5qWqsrgaJpZM4JMU7W
.
@akarnokd
Member

akarnokd commented Jul 18, 2016

I'm merging this so we can progress with other tasks that would affect the same files. I'll address the review feedback in a separate PR if necessary.

akarnokd merged commit 487a0ba into ReactiveX:2.x Jul 18, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

akarnokd deleted the akarnokd:TestSync714_1 branch Jul 18, 2016

@artem-zinnatullin artem-zinnatullin commented on the diff Jul 18, 2016

View full changes
build.gradle
}
group = 'io.reactivex.rxjava2'
description = 'RxJava: Reactive Extensions for the JVM – a library for composing asynchronous and event-based programs using observable sequences for the Java VM.'
apply plugin: 'java'
-apply plugin: 'pmd'
+// apply plugin: 'pmd'
@artem-zinnatullin

artem-zinnatullin Jul 18, 2016

Contributor

Why?

@akarnokd

akarnokd Jul 18, 2016

Member

Our build task gets killed most of the time if it is enabled.

@artem-zinnatullin

artem-zinnatullin Jul 18, 2016

Contributor

hm, that's sad

On Tue, Jul 19, 2016 at 12:33 AM, David Karnok notifications@github.com
wrote:

In build.gradle
#4204 (comment):

}

group = 'io.reactivex.rxjava2'

description = 'RxJava: Reactive Extensions for the JVM – a library for composing asynchronous and event-based programs using observable sequences for the Java VM.'

apply plugin: 'java'
-apply plugin: 'pmd'
+// apply plugin: 'pmd'

Our build task gets killed most of the time if it is enabled.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/ReactiveX/RxJava/pull/4204/files/a9752a3d697c31ad1ac46bb41891777038e0b0d8#r71234422,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA7B3CFN7hn3Q5ju6HxbFuMF08iCxKEWks5qW_ElgaJpZM4JMU7W
.

@artem-zinnatullin artem-zinnatullin commented on the diff Jul 18, 2016

View full changes
...o/reactivex/exceptions/OnCompleteFailedException.java
@@ -18,7 +18,11 @@
private static final long serialVersionUID = -6179993283427447098L;
public OnCompleteFailedException(Throwable cause) {
- super(cause);
+ super(cause != null ? cause : new NullPointerException());
@artem-zinnatullin

artem-zinnatullin Jul 18, 2016

Contributor

not sure it's a good idea, npe will point here and can confuse reader of the stacktrace.

// Also, creating npes manually is usually a bad practice, it's an exception thrown by VM, but RxJava throws them already so it's up to u

@akarnokd

akarnokd Jul 18, 2016

Member

This has been discussed back then: instead of crashing the code further, it sets an NPE with the current stack trace to give something to the receiver.

@artem-zinnatullin

artem-zinnatullin Jul 18, 2016

Contributor

ok, but I though it's ok to have null as cause…

On Tue, Jul 19, 2016 at 12:35 AM, David Karnok notifications@github.com
wrote:

In src/main/java/io/reactivex/exceptions/OnCompleteFailedException.java
#4204 (comment):

@@ -18,7 +18,11 @@
private static final long serialVersionUID = -6179993283427447098L;

 public OnCompleteFailedException(Throwable cause) {

  •    super(cause);

  •    super(cause != null ? cause : new NullPointerException());

This has been discussed back then: instead of crashing the code further,
it sets an NPE with the current stack trace to give something to the
receiver.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/ReactiveX/RxJava/pull/4204/files/a9752a3d697c31ad1ac46bb41891777038e0b0d8#r71234794,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA7B3K3Dpwpx9tC1aBXSB4f_FR2yi0iCks5qW_GjgaJpZM4JMU7W
.

@akarnokd

akarnokd Jul 18, 2016

Member

Here, it is an indication that somewhere the cause wasn't set properly, the NPE will help identify that.

@artem-zinnatullin artem-zinnatullin commented on the diff Jul 18, 2016

View full changes
...rnal/operators/flowable/BlockingFlowableIterator.java
+
+ return v;
+ }
+ throw new NoSuchElementException();
+ }
+
+ @Override
+ public void onSubscribe(Subscription s) {
+ if (SubscriptionHelper.setOnce(this, s)) {
+ s.request(batchSize);
+ }
+ }
+
+ @Override
+ public void onNext(T t) {
+ if (!queue.offer(t)) {
@artem-zinnatullin

artem-zinnatullin Jul 18, 2016

Contributor

nit: reversed if will be easier to read/maintain
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment