Re: Partial restructuring of Freenet's code
Martin Byrenheid <[email protected]> Fri, 10 May 2019 16:59:51 +0200
| Newsgroups | gmane.network.freenet.devel |
|---|---|
| Message-ID | <[email protected]> |
--------------9B7EF272FB7423866F64889C Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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=E2=80=99s not =C2=A0clear to me h= ow many > places that will be of use, or whether mocking would still be viable > in integration tests instead of just unit tests, but it=E2=80=99s defin= itely 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=E2=80=99s 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 > <[email protected] > <mailto:[email protected]>> 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]= =2E >> >> 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= =2E >> By overriding these protected methods, we can replace class members wi= th >> 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 i= ts >> 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= =2E >> To illustrate my changes, consider the following three example classes= : >> >> package example.A; >> class MyClass { >> >> =C2=A0=C2=A0=C2=A0 boolean getBool() { ... } >> >> =C2=A0=C2=A0=C2=A0 public String getString() { ... }=C2=A0=C2=A0=C2=A0= >> >> } >> >> package example.A; >> class SamePackageClass { >> =C2=A0 public SamePackageClass(MyClass ref) { >> >> =C2=A0=C2=A0=C2=A0 if (ref.getBool()) ... >> >> =C2=A0 } >> >> } >> >> package example.B; >> class OtherPackageClass { >> >> =C2=A0 public OtherPackageClass(MyClass ref) { >> >> =C2=A0=C2=A0=C2=A0 process(ref.getString()); >> >> =C2=A0 } >> >> } >> >> 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 t= he >> following 2 interfaces: >> >> package example.A; >> public interface MyClass { >> >> =C2=A0=C2=A0=C2=A0 String getString(); >> >> } >> >> package example.A; >> interface ProtectedMyClass extends MyClass { >> >> =C2=A0=C2=A0=C2=A0 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 { >> >> =C2=A0=C2=A0=C2=A0 public boolean getBool() { ... } >> >> =C2=A0=C2=A0=C2=A0 public String getString() { ... }=C2=A0=C2=A0=C2=A0= >> >> } >> >> package example.A; >> class SamePackageClass { >> =C2=A0 public SamePackageClass(ProtectedMyClass ref) { >> >> =C2=A0=C2=A0=C2=A0 if (ref.getBool()) ... >> >> =C2=A0 } >> >> } >> >> package example.B; >> class OtherPackageClass { >> >> =C2=A0 public OtherPackageClass(MyClass ref) { >> >> =C2=A0=C2=A0=C2=A0 process(ref.getString()); >> >> =C2=A0 } >> >> } >> >> The getBool-Method of the MyClassImpl-class is now public. However, if= >> instances of this class are only accessed via the interfaces it is onl= y >> 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 { >> >> =C2=A0=C2=A0=C2=A0 MyClass ref; >> >> =C2=A0=C2=A0=C2=A0 public AnotherClass(int param) { >> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ref =3D newMyClass(param); >> >> =C2=A0=C2=A0=C2=A0 } >> >> =C2=A0=C2=A0=C2=A0 protected MyClass newMyClass(int param) { >> >> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 return new MyClassImpl(param); >> >> =C2=A0=C2=A0=C2=A0 } >> >> } >> >> Instead of calling "new MyClassImpl(param)" directly in the constructo= r, >> I use a protected method newMyClass. When writing a test case, I can n= ow >> easily test AnotherClass without needing an instance of MyClassImpl. T= o >> do so, I create a minimal class MyClassStub that implements the >> MyClass-interface and derive the following class: >> >> class InstrumentedAnotherClass extends AnotherClass { >> >> =C2=A0=C2=A0=C2=A0 public InstrumentedAnotherClass(int param) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 super(param); >> =C2=A0=C2=A0=C2=A0 } >> >> =C2=A0=C2=A0=C2=A0 @Override >> =C2=A0=C2=A0=C2=A0 protected MyClass newMyClass(int param) { >> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return new MyClassStub(); >> >> =C2=A0=C2=A0=C2=A0 } >> >> } >> >> 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= =2E >> >> - 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=C3=A4t 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 >> >> > > --=20 Martin Byrenheid Scientific Assistant Technische Universit=C3=A4t 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=20 CFDE 24EB E684 D0BE E785 --------------9B7EF272FB7423866F64889C Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body bgcolor="#FFFFFF" text="#000000"> <p>Hi,</p> <p>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.</p> <p>Best regards,<br> Martin<br> </p> <p>[1] <a class="moz-txt-link-freetext" href="https://github.com/freenet/fred/pull/660">https://github.com/freenet/fred/pull/660</a><br> </p> <div class="moz-cite-prefix">On 4/26/19 1:33 PM, Steve Dougherty wrote:<br> </div> <blockquote type="cite" cite="mid:PaUbNRZJB4dEwT_xbRvJvPIe6iI_rIY07IZYJf1o_hVqI_yMHU52_qMEz8N2sZ2IuvSj7efro69l6Aj2mvWJZuOUmr4n3w-eHOpIyKzGFVw=@asksteved.com"> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> <div>Thanks for writing this! Your approaches seem reasonable to me.</div> <div><br> </div> <div>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.<br> </div> <div><br> </div> <div>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.</div> <div><br> </div> <div>- Steve</div> <div><br> </div> On Fri, Apr 26, 2019 at 6:34 AM, Martin Byrenheid <<a href="mailto:[email protected]" class="" moz-do-not-send="true">[email protected]</a>> wrote: <blockquote class="protonmail_quote" type="cite"> Hello everyone,<br> <br> I've sent this mail at the end of last year to this list but it seems<br> that it never made it to its subscribers. Thus, I'm trying again:<br> <br> <br> I started to refactor several parts of the Freenet Code to make the it<br> easier to test and understand the different parts of it in isolation.<br> Before I continue doing so, I would like to know your thoughts on my<br> approach and whether you would actually welcome these changes. You can<br> find the latest version of the partially refactored code on Github [1].<br> <br> Essentially, I did the following three changes to the Node-,<br> NodeCrypto-,PeerManager-,UdpSocketHandler-,NodeIPPortDetector- and<br> IPAddressDetector-class:<br> <br> 1. Create corresponding Interfaces for each class. This allows us to<br> replace actual class instances (e.g. of the Node class) with a<br> test-specific dummy implementation. This dummy can then be given as<br> parameter to a constructor.<br> <br> 2. Encapsulate instantiation of class members within protected methods.<br> By overriding these protected methods, we can replace class members with<br> test-specific dummy implementations.<br> <br> 3. Create getter-Methods to access class members. Many classes in<br> Freenet hold a reference to a Node-instance and some directly access its<br> members. Besides the fact that this is not a good practice, it kept me<br> from creating an interface for the node class.<br> <br> One obstacle with point (1) was that there are several cases where a<br> class calls a non-public method from another class in the same package.<br> To illustrate my changes, consider the following three example classes:<br> <br> package example.A;<br> class MyClass {<br> <br> boolean getBool() { ... }<br> <br> public String getString() { ... } <br> <br> }<br> <br> package example.A;<br> class SamePackageClass {<br> public SamePackageClass(MyClass ref) {<br> <br> if (ref.getBool()) ...<br> <br> }<br> <br> }<br> <br> package example.B;<br> class OtherPackageClass {<br> <br> public OtherPackageClass(MyClass ref) {<br> <br> process(ref.getString());<br> <br> }<br> <br> }<br> <br> Because SamePackageClass is within the same package as MyClass, the<br> getBool-method is visible to the former wheras only getString() is<br> visible for the OtherPackageClass. After my refactoring, we now have the<br> following 2 interfaces:<br> <br> package example.A;<br> public interface MyClass {<br> <br> String getString();<br> <br> }<br> <br> package example.A;<br> interface ProtectedMyClass extends MyClass {<br> <br> boolean getBool();<br> <br> }<br> <br> Please note that the first interface is a public interface while the<br> second interface is package-local. The three example classes now look as<br> follows:<br> <br> package example.A;<br> class MyClassImpl implements ProtectedMyClass {<br> <br> public boolean getBool() { ... }<br> <br> public String getString() { ... } <br> <br> }<br> <br> package example.A;<br> class SamePackageClass {<br> public SamePackageClass(ProtectedMyClass ref) {<br> <br> if (ref.getBool()) ...<br> <br> }<br> <br> }<br> <br> package example.B;<br> class OtherPackageClass {<br> <br> public OtherPackageClass(MyClass ref) {<br> <br> process(ref.getString());<br> <br> }<br> <br> }<br> <br> The getBool-Method of the MyClassImpl-class is now public. However, if<br> instances of this class are only accessed via the interfaces it is only<br> visible to classes that use the ProtectedMyClass interface. This way, we<br> maintain the package-local view for the different methods. What do you<br> think about this solution?<br> <br> <br> Given the above classes, an example for a point (2)-refactoring is as<br> follows:<br> <br> class AnotherClass {<br> <br> MyClass ref;<br> <br> public AnotherClass(int param) {<br> <br> ref = newMyClass(param);<br> <br> }<br> <br> protected MyClass newMyClass(int param) {<br> <br> return new MyClassImpl(param);<br> <br> }<br> <br> }<br> <br> Instead of calling "new MyClassImpl(param)" directly in the constructor,<br> I use a protected method newMyClass. When writing a test case, I can now<br> easily test AnotherClass without needing an instance of MyClassImpl. To<br> do so, I create a minimal class MyClassStub that implements the<br> MyClass-interface and derive the following class:<br> <br> class InstrumentedAnotherClass extends AnotherClass {<br> <br> public InstrumentedAnotherClass(int param) {<br> super(param);<br> }<br> <br> @Override<br> protected MyClass newMyClass(int param) {<br> <br> return new MyClassStub();<br> <br> }<br> <br> }<br> <br> As a proof of concept for this approach, I created some initial unit<br> tests for the NodeCrypto[2]- and NodeIPPortDetector-class[3].<br> <br> To me, two points that are still open when it comes to tests are:<br> <br> - Testing of time-based conditions: To test functionalities such as<br> rate-limiting of requests, the calls to System.currentTimeMillis()<br> probably need to be replaced by a call to a non-system-interface with a<br> similar method, whose implementation can then be adapted to test cases.<br> <br> - Concurrency: I didn't yet spend much time thinking about how classes<br> can be tested that wait for other threads to operate (e.g. location<br> swapping) but I am optimistic that this can be done.<br> <br> <br> Kind regards,<br> Martin<br> <br> <br> [1] <a class="moz-txt-link-freetext" href="https://github.com/yadevel/fred/commits/testing-refactoring">https://github.com/yadevel/fred/commits/testing-refactoring</a><br> <br> [2]<br> <a class="moz-txt-link-freetext" href="https://github.com/yadevel/fred/blob/testing-refactoring/test/freenet/node/NodeCryptoTest.java">https://github.com/yadevel/fred/blob/testing-refactoring/test/freenet/node/NodeCryptoTest.java</a><br> <br> [3]<br> <a class="moz-txt-link-freetext" href="https://github.com/yadevel/fred/blob/testing-refactoring/test/freenet/node/NodeIPPortDetectorTest.java">https://github.com/yadevel/fred/blob/testing-refactoring/test/freenet/node/NodeIPPortDetectorTest.java</a><br> <br> --<br> Martin Byrenheid<br> Scientific Assistant<br> <br> Technische Universität Dresden<br> Faculty of Computer Science<br> Institute of System Architecture<br> Chair of Privacy and Data Security<br> 01062 Dresden<br> <br> Tel.: +49 351 463 38235<br> GPG : F144 FBCD F0C2 257A 87ED CFDE 24EB E684 D0BE E785<br> <br> <br> </blockquote> <div><br> </div> <div><br> </div> </blockquote> <pre class="moz-signature" cols="72">-- 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</pre> </body> </html> --------------9B7EF272FB7423866F64889C--