Fix Persistent Connections (Issue 7230)#1121
Conversation
Unfortunately, it currently looks like we won’t be able to update to a newer version than 3, because Mockito starts using WeakReference as a type for some internal representation or other, so we can’t mock objects anymore that use it.
|
TL;DR: with All of the above fail due to a The inlining ByteBuddy mock-maker (from Knowing this, we can't use |
|
I can consistently reproduce this by swithing JVM through SDKMAN and running
|
|
The tests would only need minor adjustment to do without inline mocks. This is what fixes it for me: diff --git a/build.gradle b/build.gradle
index b6864210e9..c819e937ea 100644
--- a/build.gradle
+++ b/build.gradle
@@ -264,7 +264,7 @@ dependencies {
implementation "org.slf4j:slf4j-api:1.7.25"
testImplementation 'junit:junit:4.13.2'
- testImplementation "org.mockito:mockito-inline:2.28.2"
+ testImplementation "org.mockito:mockito-core:2.28.2"
testImplementation "org.hamcrest:hamcrest:3.0"
}
diff --git a/test/freenet/clients/http/ToadletContextImplTest.java b/test/freenet/clients/http/ToadletContextImplTest.java
index 4a4f1ee5d8..6c074c0f13 100644
--- a/test/freenet/clients/http/ToadletContextImplTest.java
+++ b/test/freenet/clients/http/ToadletContextImplTest.java
@@ -21,11 +21,10 @@ import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.equalTo;
-import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Matchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Mockito.any;
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
-import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
@@ -61,9 +60,7 @@ public class ToadletContextImplTest {
@Test
public void redirectExceptionFromToadletResultsInRedirect() throws Exception {
setupInputStream("GET /redirect-toadlet HTTP/1.0\r\n\r\n");
- Toadlet redirectingToadlet = mock(Toadlet.class, RETURNS_DEEP_STUBS);
- when(redirectingToadlet.findSupportedMethods()).thenReturn("GET");
- doThrow(new RedirectException("/new-location")).when(redirectingToadlet).handleMethodGET(any(), any(), any());
+ Toadlet redirectingToadlet = new RedirectToadlet("/new-location");
when(toadletContainer.findToadlet(new URI("/redirect-toadlet"))).thenReturn(redirectingToadlet);
when(toadletContainer.findToadlet(new URI("/new-location"))).thenReturn(homepageToadlet);
ToadletContextImpl.handle(socket, toadletContainer, null, null, null);
@@ -156,21 +153,21 @@ public class ToadletContextImplTest {
}
@Test
- public void requestWithFullAccessForwardsRequestToPageMakerForParsingAdvancedModeSwitches() throws Exception {
- setupInputStream("GET / HTTP/1.0\r\n\r\n");
+ public void requestWithFullAccessSetsAdvancedModeWhenQueryParameterIsProvided() throws Exception {
+ setupInputStream("GET /?fproxyAdvancedMode=2 HTTP/1.0\r\n\r\n");
when(toadletContainer.isAllowedFullAccess(any())).thenReturn(true);
when(toadletContainer.findToadlet(any())).thenReturn(homepageToadlet);
ToadletContextImpl.handle(socket, toadletContainer, pageMaker, null, null);
- verify(pageMaker).parseMode(any(), eq(toadletContainer));
+ verify(toadletContainer).setAdvancedMode(true);
}
@Test
- public void requestWithoutFullAccessDoesNotForwardRequestToPageMakerForParsingAdvancedModeSwitches() throws Exception {
- setupInputStream("GET / HTTP/1.0\r\n\r\n");
+ public void requestWithoutFullAccessDoesNotSetAdvancedMode() throws Exception {
+ setupInputStream("GET /?fproxyAdvancedMode=2 HTTP/1.0\r\n\r\n");
when(toadletContainer.isAllowedFullAccess(any())).thenReturn(false);
when(toadletContainer.findToadlet(any())).thenReturn(homepageToadlet);
ToadletContextImpl.handle(socket, toadletContainer, pageMaker, null, null);
- verify(pageMaker, never()).parseMode(any(), eq(toadletContainer));
+ verify(toadletContainer, never()).setAdvancedMode(anyBoolean());
}
@Test
@@ -331,30 +328,80 @@ public class ToadletContextImplTest {
private final Socket socket = mock(Socket.class, RETURNS_DEEP_STUBS);
private final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
private final ToadletContainer toadletContainer = mock(ToadletContainer.class, RETURNS_DEEP_STUBS);
- private final PageMaker pageMaker = mock(PageMaker.class, RETURNS_DEEP_STUBS);
+ private final PageMaker pageMaker = new PageMaker(PageMaker.THEME.CLEAN, null);
- private final Toadlet homepageToadlet = mock(Toadlet.class, RETURNS_DEEP_STUBS);
- private final Toadlet noOutputToadlet = mock(Toadlet.class, RETURNS_DEEP_STUBS);
+ private final Toadlet homepageToadlet = new HomepageToadlet();
+ private final Toadlet noOutputToadlet = new NoOutputToadlet();
private final PostToadlet postToadlet = new PostToadlet();
{
try {
- when(socket.getOutputStream()).thenReturn(outputStream);
+ doReturn(outputStream).when(socket).getOutputStream();
+ doReturn(null).when(socket).getInetAddress();
when(toadletContainer.getBucketFactory()).thenReturn(new ArrayBucketFactory());
-
- doAnswer(invocation -> {
- ToadletContext toadletContext = (ToadletContext) invocation.getArguments()[2];
- toadletContext.sendReplyHeaders(200, "OK", null, "text/plain", 7);
- toadletContext.writeData(new byte[] { 'G', 'E', 'T', ' ', 'O', 'K', '\n' });
- return null;
- }).when(homepageToadlet).handleMethodGET(any(), any(), any());
- when(homepageToadlet.findSupportedMethods()).thenReturn("GET");
- when(noOutputToadlet.findSupportedMethods()).thenReturn("GET");
- } catch (IOException | ToadletContextClosedException | RedirectException e) {
+ } catch (IOException e) {
throw new RuntimeException(e);
}
}
+ private static class RedirectToadlet extends Toadlet {
+
+ private final String redirectTarget;
+
+ @Override
+ public void handleMethodGET(URI uri, HTTPRequest request, ToadletContext ctx) throws RedirectException {
+ throw new RedirectException(URI.create(redirectTarget));
+ }
+
+ @Override
+ public String path() {
+ return "/redirect-toadlet";
+ }
+
+ RedirectToadlet(String redirectTarget) {
+ super(mock(HighLevelSimpleClient.class));
+ this.redirectTarget = redirectTarget;
+ }
+
+ }
+
+ private static class HomepageToadlet extends Toadlet {
+
+ @Override
+ public void handleMethodGET(URI uri, HTTPRequest request, ToadletContext ctx) throws ToadletContextClosedException, IOException {
+ ctx.sendReplyHeaders(200, "OK", null, "text/plain", 7);
+ ctx.writeData(new byte[]{'G', 'E', 'T', ' ', 'O', 'K', '\n'});
+ }
+
+ @Override
+ public String path() {
+ return "/homepage-toadlet";
+ }
+
+ HomepageToadlet() {
+ super(mock(HighLevelSimpleClient.class));
+ }
+
+ }
+
+ private static class NoOutputToadlet extends Toadlet {
+
+ @Override
+ public void handleMethodGET(URI uri, HTTPRequest request, ToadletContext ctx) {
+ // no output
+ }
+
+ @Override
+ public String path() {
+ return "/no-output";
+ }
+
+ NoOutputToadlet() {
+ super(mock(HighLevelSimpleClient.class));
+ }
+
+ }
+
private static class PostToadlet extends Toadlet {
@SuppressWarnings("unused") |
|
Godfuckingdamnit, I fucking hate Gradle. Why the fuck do I have to fight Gradle all the time?! 🤬 Anyway, it’s also possible to update the byte-buddy version to something more recent; latest Mockito version uses 1.17, but 1.18 works fine here, too. |
The reason those inline mocks are appealing is because they allow our tests to effortlessly work around poorly architected code -- but only until the next major class version (which seems to arrive every 6 months nowadays) after which our test compatibility becomes dependent on the next byte-buddy release. With this in mind, I'd rather minimize our dependency on byte-buddy and avoid inline mocks. |
…of which we have a lot… 😄
Well… yes, and no. I believe the end result still has to run on Java 8, and building/running tests is not something that the average user will do. I would even go as far as to say that if you’re building fred on a vastly more recent version than what we support, you’re on your own. 🙂
Well, as long as we’re using Mockito, byte-buddy is going nowhere. 😄 Generally you are correct, though; Mockito should actually be used sparingly (and I have started reducing my use of Mockito as the go-to solution for mocking) and only for third-party code that you can’t change. Fred consisting of basically 100% public API (of which large parts can’t be changed either) makes this a rather tricky situation. 😄 Thank you for the patch! I will take a look at at and see if I can whip it into something that I can complete agree with, because it’s not yet sitting quite right with me. 🙂 |
2 participants
(I inadvertently closed the old PR by removing the branch in my repository, so here is another one!)
Bug Report: https://freenet.mantishub.io/view.php?id=7230
This branch fixes the problem outlined in the bug report mentioned above, but it also does so much more. 😁 There are four types of commits:
🧑💻 – lays groundwork for upcoming tests. This comprises a parser for HTTP responses and Hamcrest matchers for said parsed responses, allowing more expressive tests.
⬆️ – updates dependencies. This updates to the last version of Mockito that we can use (due to its inability to mock anything involving
WeakReferencestarting in version 3), allowing mocks of final classes and classes with final methods; required for mockingToadlet.findSupportedMethods()!🐛 – fixes problems I found in the
ToadletContextImpl. One of them is the one from the bug report, the other ones are small issues that I found during tests.✅ – adds additional tests.
The final barrage of tests achieves almost full coverage of the outer-most try block in
ToadletContextImpl.handle().