SEPURI-SAI-KRISHNA opened a new pull request, #12410: URL: https://github.com/apache/seatunnel/pull/12410
### Purpose of this pull request Follow-up to @DanielLeens's review on #12406, covering Issues 1 and 3 from it. Test-only; the production class is untouched. **Issue 1, the assertion was too weak.** The round-trip test added in #12406 asserted `source.getPluginName()` against `restored.getPluginName()`. That is a constant, so the test would have passed even if every field had been lost in serialization, which is exactly the failure it exists to catch. Two changes: * `MqttSourceTest.testSourceIsJavaSerializable` now asserts restored state, comparing the produced catalog table's id and schema and checking boundedness, instead of a constant. * A round trip of `MqttSourceConfig` itself is added to the existing `MqttSourceConfigTest`. That is the class #12406 changed, and unlike `MqttSource` it exposes getters, so the assertion can check the values directly. The config test deliberately uses values that differ from the defaults, so a restored object that silently rebuilt its defaults would not satisfy it. Worth noting that the only legal non-default `qos` is `0`: the default is `1` and `validate()` rejects anything above `1`. My first attempt used `2` and was correctly rejected by the connector's own validation. **Issue 3, the test did not mirror the engine's ordering.** The engine sets the job context before serializing the logical DAG. The test serialized a source that had never had one. It now calls `setJobContext` first. ### Does this PR introduce _any_ user-facing change? No. Two test classes, both extended rather than created. No production code. ### How was this patch tested? `./mvnw -q -DskipTests verify -pl seatunnel-connectors-v2/connector-mqtt` on JDK 11 passes, covering the enforcer checks, `spotless:check` and compilation. `spotless:check` was also run separately with up-to-date caching disabled. `./mvnw -pl seatunnel-connectors-v2/connector-mqtt test`: **33 tests, 0 failures, 0 errors**. Since the point of this PR is that an assertion was not actually catching anything, I checked that these ones do: * making `url` `transient` in `MqttSourceConfig` fails `testConfigSurvivesJavaSerialization`, and the previous constant-only assertion would not have noticed * removing `implements Serializable` altogether fails both `testConfigSurvivesJavaSerialization` and `testSourceIsJavaSerializable` Issue 2 from that review, the missing `MqttSourceIT`, is opened separately as #12408. It needs a new IT class and a source job against the broker container `MqttSinkIT` already stands up, so it did not belong in the same change as these two assertions. -- 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]
