david-streamlio commented on PR #22: URL: https://github.com/apache/pulsar-connectors/pull/22#issuecomment-4407540308
**Suggestion: split unit tests (Mockito) from integration tests (TestContainers)** `MqttSinkTest` currently does two different jobs in one file: - It stands up a real `eclipse-mosquitto:2` broker via TestContainers and runs a full publish/subscribe round‑trip — that's an **integration test**. - It uses `mock(SinkContext.class)` from Mockito — that's a **unit‑test** idiom. Because the class is named `*Test`, Gradle's default `test` task will execute it on every build, which means `./gradlew test` now requires a running Docker daemon for every contributor and every CI lane. That's a slow and brittle default for what should be the fast feedback loop. I'd suggest separating the two concerns: **1. `MqttSinkTest` — pure unit tests with Mockito (no Docker).** Cover the connector's own logic in isolation by mocking `Mqtt5AsyncClient` (and `SinkContext`, `Record`). Good candidates: - `record.ack()` is invoked when the publish `CompletableFuture` completes successfully. - `record.fail()` is invoked when the publish future completes exceptionally, and on the synchronous catch path in `write()`. - A `null` payload is normalized to `new byte[0]` before publishing. - Config validation failures from `MqttSinkConfig#validate` propagate out of `open()`. - `close()` is a safe no‑op when `open()` was never called or failed mid‑way. - Username present → `simpleAuth()` branch; username blank → plain connect branch. To make these mockable, `MqttSink#open` would benefit from a small seam — e.g. a package‑private `Mqtt5AsyncClient buildClient(MqttSinkConfig)` method, or a `MqttClientFactory` injected via constructor — so tests can substitute a mock without PowerMock or reflection. **2. `MqttSinkIntegrationTest` (or `MqttSinkIT`) — TestContainers + real Mosquitto.** Keep the current end‑to‑end scenario here. This is where TestContainers shines: it validates the wire protocol, QoS semantics, auth, TLS, reconnect behavior — things mocks can't catch. Run it via a dedicated source set / Gradle task (e.g. `integrationTest`) so it stays out of the default `test` lane and only runs where Docker is available. Concretely in `mqtt/build.gradle.kts`: - Add `testImplementation(libs.mockito.core)` (and `mockito-junit-jupiter` / `mockito-testng` to match the test runner). - Add an `integrationTest` source set + task, move `testcontainers` to `integrationTestImplementation`, and wire it into CI separately from `test`. Net effect: `./gradlew test` stays fast and Docker‑free for day‑to‑day development, while `./gradlew integrationTest` gives you the real‑broker confidence you currently get from `MqttSinkTest`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
