MockWebServer extension is starting new server when I call @BeforeEach and @AfterEach

[garcia-jj]

The MockWebServerExtension class is starting a new MockWebServer for every test, beforeEach, beforeAll, afterEach and beforeAll methods, as I can see in this class.

Since the beforeEach, beforeAll, afterEach and beforeAll methods are usually used just to configure the tests, I want to suggest to use the same server instance when run before/after methods, creating a new fresh instance only for test methods.

What do you think?

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

import mockwebserver3.MockWebServer;
import mockwebserver3.junit5.internal.MockWebServerExtension;

@ExtendWith(MockWebServerExtension.class)
public class MyServerTest {

	@BeforeEach
	public void setup(final MockWebServer server) {
		System.out.println("Port:" + server.getPort());
	}

	@Test
	public void myTest(final MockWebServer server) throws Exception {
		System.out.println("Port:" + server.getPort());

	}
}

[yschimke]

Thanks, as a workaround pass in the MockWebServer in the test constructor. Changing to enhancement as it's not clear this is against the Junit 5 spec, and given the workaround it may not be the highest priority relative to other tasks.

But I think you are right, without any other annotations we should probably reuse the instance.

Seem like in https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/extension/ParameterResolver.html we can check the https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/extension/ParameterContext.html for an annotation that separates instances when we want multiple, but that shouldn't be the default.

[garcia-jj]

Thank you so much, @yschimke. Adding a constructor as you suggested solves my case. I'll looking forward this enhacement for more updates.

[swankjesse]

I'd prefer to not reuse the instance by default. We don't reuse it that in OkHttp’s test suite and it's never been a performance problem.

[JakeWharton]

OkHttp's test suite does not behave this way because it uses JUnit 4 where the instance is shared by the before, test, and after methods.

This issue proposes bringing it in line with that behavior rather than creating a new instance for each lifecycle method of a single test execution.

[garcia-jj]

Before/After methods are not exactly test methods, so in my opinion needs to have the same instance as test methods. But, of course, all test methods sould have a new instance, as @swankjesse suggested.

[yschimke]

OK, and probably fail if you try to use this on BeforeAll/AfterAll as that would encourage using a shared instance.

[yschimke]

I think to do this right, we need to drop support for constructors and statics, like BeforeAll.

See https://github.com/junit-team/junit5/issues/1746 which changes the policy for TempDir because it's confusing otherwise.

[diegozklein]

The MockWebServerExtension class is starting a new MockWebServer for every test, beforeEach, beforeAll, afterEach and beforeAll methods, as I can see in this class.

Since the beforeEach, beforeAll, afterEach and beforeAll methods are usually used just to configure the tests, I want to suggest to use the same server instance when run before/after methods, creating a new fresh instance only for test methods.

What do you think?

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

import mockwebserver3.MockWebServer;
import mockwebserver3.junit5.internal.MockWebServerExtension;

@ExtendWith(MockWebServerExtension.class)
public class MyServerTest {

	@BeforeEach
	public void setup(final MockWebServer server) {
		System.out.println("Port:" + server.getPort());
	}

	@Test
	public void myTest(final MockWebServer server) throws Exception {
		System.out.println("Port:" + server.getPort());

	}
}

This would actually be very useful

[yschimke]

I have a few test foxes to land in https://github.com/square/okhttp/pull/7392

[yschimke]

Hopefully working now, reviews welcome, particularly from Junit5 aspect.