Thanks for writing this! Your approaches seem reasonable to me.
Yes, mocking time will be helpful. It’s not clear to me how many places that
will be of use, or whether mocking would still be viable in integration tests
instead of just unit tests, but it’s definitely a good tool to have. Do you
know if JUnit has anything that can help with that? It seems like a common
enough need that it might.
Testing concurrent things may require more Conditions for threads to wait on.
It’s not clear to me how much effort that will require to be viable, or how
best to expose them.
- Steve
On Fri, Apr 26, 2019 at 6:34 AM, Martin Byrenheid
<martin.byrenh...@tu-dresden.de> wrote:
> Hello everyone,
>
> I've sent this mail at the end of last year to this list but it seems
> that it never made it to its subscribers. Thus, I'm trying again:
>
> I started to refactor several parts of the Freenet Code to make the it
> easier to test and understand the different parts of it in isolation.
> Before I continue doing so, I would like to know your thoughts on my
> approach and whether you would actually welcome these changes. You can
> find the latest version of the partially refactored code on Github [1].
>
> Essentially, I did the following three changes to the Node-,
> NodeCrypto-,PeerManager-,UdpSocketHandler-,NodeIPPortDetector- and
> IPAddressDetector-class:
>
> 1. Create corresponding Interfaces for each class. This allows us to
> replace actual class instances (e.g. of the Node class) with a
> test-specific dummy implementation. This dummy can then be given as
> parameter to a constructor.
>
> 2. Encapsulate instantiation of class members within protected methods.
> By overriding these protected methods, we can replace class members with
> test-specific dummy implementations.
>
> 3. Create getter-Methods to access class members. Many classes in
> Freenet hold a reference to a Node-instance and some directly access its
> members. Besides the fact that this is not a good practice, it kept me
> from creating an interface for the node class.
>
> One obstacle with point (1) was that there are several cases where a
> class calls a non-public method from another class in the same package.
> To illustrate my changes, consider the following three example classes:
>
> package example.A;
> class MyClass {
>
> boolean getBool() { ... }
>
> public String getString() { ... }
>
> }
>
> package example.A;
> class SamePackageClass {
> public SamePackageClass(MyClass ref) {
>
> if (ref.getBool()) ...
>
> }
>
> }
>
> package example.B;
> class OtherPackageClass {
>
> public OtherPackageClass(MyClass ref) {
>
> process(ref.getString());
>
> }
>
> }
>
> Because SamePackageClass is within the same package as MyClass, the
> getBool-method is visible to the former wheras only getString() is
> visible for the OtherPackageClass. After my refactoring, we now have the
> following 2 interfaces:
>
> package example.A;
> public interface MyClass {
>
> String getString();
>
> }
>
> package example.A;
> interface ProtectedMyClass extends MyClass {
>
> boolean getBool();
>
> }
>
> Please note that the first interface is a public interface while the
> second interface is package-local. The three example classes now look as
> follows:
>
> package example.A;
> class MyClassImpl implements ProtectedMyClass {
>
> public boolean getBool() { ... }
>
> public String getString() { ... }
>
> }
>
> package example.A;
> class SamePackageClass {
> public SamePackageClass(ProtectedMyClass ref) {
>
> if (ref.getBool()) ...
>
> }
>
> }
>
> package example.B;
> class OtherPackageClass {
>
> public OtherPackageClass(MyClass ref) {
>
> process(ref.getString());
>
> }
>
> }
>
> The getBool-Method of the MyClassImpl-class is now public. However, if
> instances of this class are only accessed via the interfaces it is only
> visible to classes that use the ProtectedMyClass interface. This way, we
> maintain the package-local view for the different methods. What do you
> think about this solution?
>
> Given the above classes, an example for a point (2)-refactoring is as
> follows:
>
> class AnotherClass {
>
> MyClass ref;
>
> public AnotherClass(int param) {
>
> ref = newMyClass(param);
>
> }
>
> protected MyClass newMyClass(int param) {
>
> return new MyClassImpl(param);
>
> }
>
> }
>
> Instead of calling "new MyClassImpl(param)" directly in the constructor,
> I use a protected method newMyClass. When writing a test case, I can now
> easily test AnotherClass without needing an instance of MyClassImpl. To
> do so, I create a minimal class MyClassStub that implements the
> MyClass-interface and derive the following class:
>
> class InstrumentedAnotherClass extends AnotherClass {
>
> public InstrumentedAnotherClass(int param) {
> super(param);
> }
>
> @Override
> protected MyClass newMyClass(int param) {
>
> return new MyClassStub();
>
> }
>
> }
>
> As a proof of concept for this approach, I created some initial unit
> tests for the NodeCrypto[2]- and NodeIPPortDetector-class[3].
>
> To me, two points that are still open when it comes to tests are:
>
> - Testing of time-based conditions: To test functionalities such as
> rate-limiting of requests, the calls to System.currentTimeMillis()
> probably need to be replaced by a call to a non-system-interface with a
> similar method, whose implementation can then be adapted to test cases.
>
> - Concurrency: I didn't yet spend much time thinking about how classes
> can be tested that wait for other threads to operate (e.g. location
> swapping) but I am optimistic that this can be done.
>
> Kind regards,
> Martin
>
> [1] https://github.com/yadevel/fred/commits/testing-refactoring
>
> [2]
> https://github.com/yadevel/fred/blob/testing-refactoring/test/freenet/node/NodeCryptoTest.java
>
> [3]
> https://github.com/yadevel/fred/blob/testing-refactoring/test/freenet/node/NodeIPPortDetectorTest.java
>
> --
> Martin Byrenheid
> Scientific Assistant
>
> Technische Universität Dresden
> Faculty of Computer Science
> Institute of System Architecture
> Chair of Privacy and Data Security
> 01062 Dresden
>
> Tel.: +49 351 463 38235
> GPG : F144 FBCD F0C2 257A 87ED CFDE 24EB E684 D0BE E785
