Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fix tests for updated Schema Registry client internals #9982

Merged
merged 1 commit into from Jun 23, 2023

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Jun 22, 2023

Description

Some unit tests are failing in the build after confluentinc/schema-registry#2674 was merged, due to improper usage of mock SR clients on the ksql side. We should be using the provided MockSchemaRegistryClient implementation rather than Mockito.mock(SchemaRegistryClient.class) since the methods which need to be mocked when using the latter are implementation details of SR which are subject to change.

The specific failure in this case is that a new internal method added to the SchemaRegistryClient usage is resulting in an NPE in our unit tests because the unit tests do not provide mocked implementations for this new method. Here's an example failure from KsqlJsonSchemaDeserializerTest.shouldDeserializeJsonObjectCorrectly:

Error serializing message to topic: bob. Converting Kafka Connect data to byte[] failed due to serialization error of topic bob: 
Hint: You probably forgot to add VALUE_SCHEMA_ID when creating the source.
	at io.confluent.ksql.serde.connect.KsqlConnectSerializer.serialize(KsqlConnectSerializer.java:56)
	at io.confluent.ksql.serde.tls.ThreadLocalSerializer.serialize(ThreadLocalSerializer.java:37)
	at io.confluent.ksql.serde.json.KsqlJsonSchemaDeserializerTest.shouldDeserializeJsonObjectCorrectly(KsqlJsonSchemaDeserializerTest.java:109)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.mockito.internal.runners.DefaultInternalRunner$1$1.evaluate(DefaultInternalRunner.java:55)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.mockito.internal.runners.DefaultInternalRunner$1.run(DefaultInternalRunner.java:100)
	at org.mockito.internal.runners.DefaultInternalRunner.run(DefaultInternalRunner.java:107)
	at org.mockito.internal.runners.StrictRunner.run(StrictRunner.java:41)
	at org.mockito.junit.MockitoJUnitRunner.run(MockitoJUnitRunner.java:163)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
	at org.junit.vintage.engine.execution.RunnerExecutor.execute(RunnerExecutor.java:42)
	at org.junit.vintage.engine.VintageTestEngine.executeAllChildren(VintageTestEngine.java:80)
	at org.junit.vintage.engine.VintageTestEngine.execute(VintageTestEngine.java:72)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:107)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
	at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
	at org.apache.maven.surefire.junitplatform.LazyLauncher.execute(LazyLauncher.java:55)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.execute(JUnitPlatformProvider.java:223)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:175)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:139)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:456)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:169)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:595)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:581)
Caused by: org.apache.kafka.connect.errors.DataException: Converting Kafka Connect data to byte[] failed due to serialization error of topic bob: 
	at io.confluent.connect.json.JsonSchemaConverter.fromConnectData(JsonSchemaConverter.java:106)
	at io.confluent.connect.json.JsonSchemaConverter.fromConnectData(JsonSchemaConverter.java:87)
	at io.confluent.ksql.serde.connect.KsqlConnectSerializer.serialize(KsqlConnectSerializer.java:53)
	... 49 more
Caused by: org.apache.kafka.common.errors.SerializationException: Error serializing JSON message
	at io.confluent.kafka.serializers.json.AbstractKafkaJsonSchemaSerializer.serializeImpl(AbstractKafkaJsonSchemaSerializer.java:166)
	at io.confluent.connect.json.JsonSchemaConverter$Serializer.serialize(JsonSchemaConverter.java:175)
	at io.confluent.connect.json.JsonSchemaConverter.fromConnectData(JsonSchemaConverter.java:98)
	... 51 more
Caused by: java.lang.NullPointerException
	at io.confluent.kafka.schemaregistry.client.rest.entities.Schema.<init>(Schema.java:169)
	at io.confluent.kafka.serializers.AbstractKafkaSchemaSerDe.registerWithResponse(AbstractKafkaSchemaSerDe.java:507)
	at io.confluent.kafka.serializers.json.AbstractKafkaJsonSchemaSerializer.serializeImpl(AbstractKafkaJsonSchemaSerializer.java:126)
	... 53 more

This PR fixes the failing tests by updating our mock SR client usage to MockSchemaRegistryClient.

This PR also contains an additional fix for the SR dependency version: it was set to 7.6.0-0 in this PR which is a mistake -- setting the version to a -0 nanoversion means that Jenkins no longer automatically bumps the version as new nanoversions are released (here's an example of where this previously happened), which means ksql development locally will not match what's seen in Jenkins, since Jenkins continually recompiles the latest schema-registry whereas ksql developers typically do not.

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")
  • Do these changes have compatibility implications for rollback? If so, ensure that the ksql command version is bumped.

@vcrfxia
Copy link
Contributor Author

vcrfxia commented Jun 23, 2023

PR build made it past the test failures that this PR intends to fix. There are other unrelated failures in the ksql master build right now which failed the build. Going to merge to improve the situation even though it's still not green yet.

@vcrfxia vcrfxia merged commit 1c89794 into confluentinc:master Jun 23, 2023
2 of 3 checks passed
@vcrfxia vcrfxia deleted the mock-sr-client branch June 23, 2023 15:38
aliehsaeedii added a commit that referenced this pull request Jul 6, 2023
…#10003)

Co-authored-by: Victoria Xia <victoria.xia@confluent.io>
lucasbru pushed a commit to lucasbru/ksql that referenced this pull request Jul 11, 2023
lucasbru added a commit that referenced this pull request Jul 12, 2023
Co-authored-by: Victoria Xia <victoria.xia@confluent.io>
Co-authored-by: asaeedi <asaeedi@confluent.io>
Co-authored-by: Alieh <107070585+aliehsaeedii@users.noreply.github.com>
Co-authored-by: Hao Li <1127478+lihaosky@users.noreply.github.com>
fix tests for updated Schema Registry client internals (#9982) (#10003)
fix: implement default method for schema registry client (#10000)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants