Add delaySubscription() methods to Completable #5081

[soshial]

Since Observable, Single already have delaySubscription(), but Completable doesn't, I added these methods to the code.

[akarnokd]

Please fix the mistakes and add unit tests verifying these new methods.

[soshial]

The code failed this test ParamValidationCheckerTest > checkCompletable FAILED.

Which should I better do:

1. put ObjectHelper.verifyPositive(delay, "delay"); into all new methods of mine or
2. return RxJavaPlugins.onAssembly(new CompletableTimer(Math.max(delay, 0L), unit, scheduler)); inside timer(...)

The first way looks more logical to me.

[akarnokd]

Neither. Update the test to permit any integer like the other delaySubscription implementations, e.g., https://github.com/ReactiveX/RxJava/blob/2.x/src/test/java/io/reactivex/validators/ParamValidationCheckerTest.java#L149

[soshial]

Got it. I will fix the tests. What about timer(), which way is correct:

https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/Completable.java#L909
or
https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/Observable.java#L3906
?

[akarnokd]

You don't have to max, the one in Observable version is old and unnecessary.

[soshial]

Should I remove max then from Observable?

Can I add @since 2.3 to the new methods?

[akarnokd]

Should I remove max then from Observable?

Sure.

Can I add @SInCE 2.3 to the new methods?

Add @since 2.2.3 - experimental and @Experimental

[codecov]

Codecov Report

Merging #6242 into 2.x will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.x    #6242      +/-   ##
============================================
+ Coverage     98.25%   98.29%   +0.04%     
- Complexity     6199     6202       +3     
============================================
  Files           667      667              
  Lines         44887    44889       +2     
  Branches       6216     6216              
============================================
+ Hits          44104    44125      +21     
+ Misses          243      239       -4     
+ Partials        540      525      -15

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/io/reactivex/Completable.java	100% <100%> (ø)	119 <2> (+2)	⬆️
...tivex/internal/observers/FutureSingleObserver.java	94.33% <0%> (-3.78%)	24% <0%> (-1%)
...reactivex/internal/operators/single/SingleAmb.java	96.36% <0%> (-3.64%)	9% <0%> (-1%)
...ternal/operators/observable/ObservablePublish.java	96.46% <0%> (-2.66%)	11% <0%> (+1%)
...va/io/reactivex/internal/queue/SpscArrayQueue.java	97.61% <0%> (-2.39%)	22% <0%> (-1%)
...ernal/operators/flowable/FlowableFlatMapMaybe.java	91.3% <0%> (-1.45%)	2% <0%> (ø)
...vex/internal/operators/parallel/ParallelRunOn.java	96.61% <0%> (-0.97%)	8% <0%> (ø)
...ava/io/reactivex/processors/BehaviorProcessor.java	98.2% <0%> (-0.9%)	59% <0%> (-1%)
...nternal/operators/observable/ObservableCreate.java	98.29% <0%> (-0.86%)	2% <0%> (ø)
...ex/internal/operators/flowable/FlowableReplay.java	94.26% <0%> (-0.85%)	20% <0%> (ø)
... and 22 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17f6e84...cb5e8cd. Read the comment docs.

[akarnokd]

Please add unit tests that verify the expected behavior.

[akarnokd]

No need to talk to a mocked observer, TestObserver.assertEmpty() already verifies these.

[akarnokd]

No need to talk to a mocked observer, TestObserver.assertEmpty() already verifies these.

[akarnokd]

No need to talk to a mocked observer, TestObserver.assertEmpty() already verifies these.

[akarnokd]

to.assertResult(); will check for the no-error condition.

[soshial]

Thanks a lot for explanations! May I simply edit the last commit, because I gave a wrong title to the commit with tests (8233972)?

[akarnokd]

Just commit the changes as fresh.

[artem-zinnatullin]

nit: here we rely just on completion signal, which as an implementation bug may be emitted by delaySubscription operator. I mean, right now there is obviously no bug, but in future if operator will be re-implemented, it might appear

Thus I'd recommend to add verifiable logic to the upstream, like Completable.fromAction { if (!atomicBoolean.compareAndSet(false, true)) throw IllegalStateException() }

and verify against that too:

to.assertResult()
assertTrue(atomicBoolean.get())

[akarnokd]

I'll add the extra tests in a separate PR.

[soshial]

Thank you, @akarnokd

[akarnokd]

@soshial do you want to address @artem-zinnatullin 's review?