rewrite e2e tests #1796
rewrite e2e tests #1796
Conversation
moscicky
force-pushed
the
buffer-loading-test
branch
3 times, most recently
from
0b91b86
to
934e1e9
Compare
- buffer loading test - graceful shutdown test - https client publishing test - publishing on avro topic without schema test
moscicky
force-pushed
the
buffer-loading-test
branch
from
934e1e9
to
b97cba2
Compare
| private static final String EXISTING_TOPIC_URL = TOPICS_ROOT + EXISTING_TOPIC; | ||
| private static final String NOT_EXISTING_TOPIC = "not-existing"; | ||
| private static final String NOT_EXISTING_TOPIC_URL = TOPICS_ROOT + NOT_EXISTING_TOPIC; | ||
| private static final int frontendPort = Ports.nextAvailable(); |
There was a problem hiding this comment.
I wouldn't use Ports.nextAvailable(). Take a look at how it is implemented. It opens and closes a socket, potentially leaving it in the TIME_WAIT state. This, in turn, sometimes leads to "Address already in use" errors. Instead, I'd use ephemeral ports - just pass 0 as a port.
There was a problem hiding this comment.
good catch, applied the suggestion in another test
| @BeforeEach | ||
| public void beforeEach() { | ||
| frontend = new HermesFrontendTestApp(infra.hermesZookeeper(), infra.kafka(), infra.schemaRegistry()); | ||
| frontend.withProperty(FRONTEND_GRACEFUL_SHUTDOWN_ENABLED, false); |
There was a problem hiding this comment.
If I'm not mistaken, this parameter doesn't affect the tests in this class in any way. Yet it is a bit misleading to set it to true while we are testing that the graceful shutdown is enabled. Perhaps, we can skip setting it.
There was a problem hiding this comment.
agree, I've set it by default as it was in old e2e tests and removed it from this test
| import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor; | ||
| import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; | ||
|
|
||
| public class AsyncPublisherAutoRetryTest { |
There was a problem hiding this comment.
What do you think about moving this test to the hermes-client module? Take a look at pl.allegro.tech.hermes.client.ReactiveHermesSenderTest. This one can also be considered an integration test but is placed in the hermes-client module.
The other way around approach is also possible, of course, but I lean towards having only tests in the integration-tests module that span multiple hermes modules.
There was a problem hiding this comment.
Looking more into this I wonder if this test is needed at all. In hermes-client module we already have tests covering cases tested here. I'd vote for removing this test class completely
Rewrite tests which require custom frontend setup:
Additionally reuse kafka, zk and schema registry between those tests