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

nullify server object on shutdown #66

Merged
merged 8 commits into from Feb 3, 2021

Conversation

nate-nutmeg
Copy link
Contributor

Nulls the server on shutdown to allow future tests to create a new server instance.

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

Copy link
Contributor

@fnobilia fnobilia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the PR @nate-nutmeg!

Can we add two more tests showing that

  1. a flow of start, stop and start will provide access to the same schemata in case the Kafka cluster is not cleaned it up
  2. a flow of start, stop and start will not provide access to the same schemata in case the Kafka cluster is cleaned it up

@nate-nutmeg
Copy link
Contributor Author

I am not sure about SchemaRegistryTestResourceLifecycleTest now that we have created it.

I feel that we might be testing more than schemaregistry-junit at this point. The test shouldBeAbleToAccessSchemasAfterSchemaRegistryTestResourceRestart is basically testing that schema-registry is able to read schemas from the _schemas topic. This is testing the functionality of schema-registry over testing the functionality of schemaregistry-junit.

Similarily with the next test. Testing that we can't access schemas after deleting the _schemas topic on a new SchemaRegistryTestResource. This feels more like testing schema-registry again. I can't get this to work strangely though. But none of the code is ours.

The last test shouldNotBeAbleToAccessSchemasOnEmptySchemasTopicWithSeperateKafkaInstances uses a new Kafka instance. It uses a hack of passing a null ExecutionContext to the beforeAll. But this just tests that the schema-registry is able to create a _schemas topic on a new kafka cluster twice in one test. I not sure it has much bearing on testing SchemaRegistryTestResource

I will leave it up to you if you want to keep the test

@fnobilia
Copy link
Contributor

@nate-nutmeg I think we are not testing the Schema Registry source code, but we are testing that we don't do anything more than starting or stopping the server when calling start or stop functions. Tests are showing that we need to alter Kafka Cluster state to change Schema Registry storage behaviour.

Let's fix the compilation issue and then it is good to go! 🚀

Thx again for the contribution! ☺

@codeclimate
Copy link

codeclimate bot commented Feb 2, 2021

Code Climate has analyzed commit 3d8551d and detected 0 issues on this pull request.

View more on Code Climate.

@fnobilia
Copy link
Contributor

fnobilia commented Feb 3, 2021

The issue with Sonarcloud cannot be solved. I tried multiple solutions suggested in a couple of Stackoverflow, but no luck. Regression testing, example tests and tests on the main code are working. I'll take the "risk" to merge into master without Sonarcloud passing.

Thx @nate-nutmeg for your contribution.

@fnobilia fnobilia merged commit 101580c into data-rocks-team:main Feb 3, 2021
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