Hi, I did not yet find the time to implement test cases for time-based or concurrent functionalities but it is still on my list for the near future (next 1-3 weeks). However, I already created interfaces and test cases for several classes. Since quite a few changes were necessary for that, I created a merge request for the current state of my code [1], so that you can let me know if you are fine with it.
Best regards, Martin [1] https://github.com/freenet/fred/pull/660 On 4/26/19 1:33 PM, Steve Dougherty wrote: > 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 > <mailto: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 >> >> > > -- 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
