Switch to PrivateConstructorChecker for "testNotInstantiable" tests

[artem-zinnatullin]

@akarnokd as I promised in #3112 I am adding nice class that allows to assert that:

• Class has only one constructor without args and that it's private.
• Constructor throws exception (optional) with some type and/or message.

Here is the repository: https://github.com/pushtorefresh/java-private-constructor-checker

Hope you'll find it nice & useful!

Previous solution didn't check that constructor is private, that class has only one constructor and it required a lot of boilerplate code!

[akarnokd]

I'm not sure about this; it is unusual to add dependencies to RxJava.

[artem-zinnatullin]

It's a small (only one class), test dependency that won't affect project.

@benjchristensen what is your opinion?

[benjchristensen]

I question the need of this assertion, thus it doesn't seem a strong enough value to warrant a dependency.

[akarnokd]

Instead of another library, you could create a utility method on a test class that does this check and call it from all places.

[artem-zinnatullin]

I can just copy PrivateConstructorChecker to the RxJava tests. You won't have problems with licence because author of PrivateConstructorChecker is me :)

Okay?

[akarnokd]

Sounds good.

[artem-zinnatullin]

Added PrivateConstructorChecker to the test utils and switched all previous testNotInstantiable tests to this checker.

@akarnokd I'll add missing tests for other non-instantiable classes in a separate PR to make it easier for review.

[benjchristensen]

I don't understand why we need this assertion, or the ability to assert this via tests.

[artem-zinnatullin]

@benjchristensen because we are humans and we make mistakes, somebody may delete private constructor in some huge PR and it can be merged and API will become public.

Quote from https://github.com/pushtorefresh/java-private-constructor-checker

Goal
Max code coverage!
It means, that you need to test even private constructors of classes that must not be instantiated.
Q: Why would I want to test that constructor is private and that it throws exception??
A: Because if you not the only developer in the project — others may change this (make constructor public, etc) and you may miss this on the code review because you are human).
A2: Because if you want to have as max code coverage as possible — you need to test even such "unreacheable" code.

[vbauer]

👍

[benjchristensen]

Without having an external dependency, that removes my strong opinion. Now I don't have strong opinions one way or another on this. I can't think of a single time when a private constructor becoming public has been an issue, nor do I think people will ever remember to write unit tests asserting this behavior, but if it is deemed worthwhile by others, it is fine by me.

@ReactiveX/rxjava-committers what do you think?

[akarnokd]

I loved to see the coverage max out with this kind of test in my related PRs and I planned to factor out the common check in the next improvement run. I'm in favor, but the checker's package should be rx.internal.util instead.

[artem-zinnatullin]

@akarnokd I hope you mean rx.internal.util in the Tests? Or you mean rx.internal.util in the main source set? If second — I don't think it's a good idea to deliver test util class to the clients, it'll make .jar bigger and add more methods which is not very good for Android.

[akarnokd]

Keep it among the tests of course.

[artem-zinnatullin]

@akarnokd done!

[stealthcode]

Is the motivation here to get a higher score on code coverage reports? Why is it necessary to create private constructors? 100% is not achievable nor is it an effective measure of good tests.

[artem-zinnatullin]

@stealthcode not only higher score on code coverage, but also verification of the class contract.

Would you test public or protected constructor or static factory? I guess yes. So why not test private constructor? It's a method of a class, it has behavior and this behavior should be tested (my personal opinion).

[stealthcode]

Okay. I can't see any harm (or benefit) from asserting that the Actions, Functions, Observers, and Subscribers classes cannot be instantiated. I personally don't think these contracts are at risk of changing. That said, it couldn't hurt 😃 Plus having the utility class could help in areas (i.e. the rx.Observable private constructor).

[artem-zinnatullin]

Fair enough :)

I'm going to add such tests for all other classes (rx.Observable too) with private constructors in a separate PR.

[stealthcode]

I don't want to discourage writing good tests. Submitting PRs to improve the quality of tests is certainly welcome and that's what I see that this PR is doing. That said, the tests that you are modifying are making assertions and establishing certain contracts that are not necessarily essential or permanent.

We are making a conscious decision not to require code coverage (or other such tools) in the release process, nor are we taking a stance on which tools we want people to use. However we encourage and hope to provide a mechanism by which contributors can standardize tools configuration if you the individual developer choose to use these tools (see #3089).

A larger decision should be made as to how we will address the use of build or bug finding tools in RxJava. I'm going to open an issue so that people can comment with suggestions or requests.

[stealthcode]

I'd support merging the helper utility if we can use it to assert that public constructors don't exist for the classes where we have static method helpers to build them. I think this list includes:

• rx.(Observable | Notification)
• rx.observables.(BlockingObservable | ConnectableObservable | GroupedObservable)
• rx.plugins.RxJavaPlugins
• rx.subjects.(AsyncSubject | BehaviorSubject | PublishSubject | ReplaySubject)

and (unless someone objects) I think we could delete the tests to assert non-public constructors for

• rx.observers.(Subscribers | Observers)
• rx.functions.(Functions | Actions)

I think these are valid contract assertions (but anyone feel free to correct me).

Also I opened the issue to discuss tools (as mentioned in my previous comment). Please see #3164.

[stealthcode]

@artem-zinnatullin can this be closed or is there still work to be done in this pull request?

[artem-zinnatullin]

Let's close this, looks like not a lot of people see sense in this (it's ok, no problem).