Skip to content

Conversation

@libretto
Copy link
Collaborator

About this change - What it does

References: #xxxxx

Why this way

@github-actions
Copy link

github-actions bot commented Jul 3, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/karapace
  schema_models.py 208-221, 240-251
  schema_reader.py 620-621, 623-628
  schema_registry_apis.py 1054
  version.py
  src/karapace/coordinator
  master_coordinator.py
  schema_coordinator.py
Project Total  

This report was generated by python-coverage-comment-action

if dependencies:
merged_schema = ""
for dependency in dependencies.values():
merged_schema += self.builder(dependency.schema.schema_str, dependency.schema.dependencies) + ",\n"

Choose a reason for hiding this comment

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

Can we please change this to a loop based approach supported by a stack or a queue, (depending on the propagation). This is to safe guard against a large nested schema.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@parislarkins
Copy link

Would it be possible to add new unit tests to cover the changes in schema_models.py, schema_reader.py, schema_registry_apis.py? Seems only integration tests have been added for that stuff

@libretto
Copy link
Collaborator Author

Would it be possible to add new unit tests to cover the changes in schema_models.py, schema_reader.py, schema_registry_apis.py? Seems only integration tests have been added for that stuff

Yes, it is possible. But creating tests may become an endless process if we do not follow the Karapace test practice. They have no unit tests for schema_registry_api.py, and the tests for schema_models and schema_reader do not look meticulous. However, they have good test coverage for the functional parts of schema storing, parsing, compatibility, etc. So we focused on these tests and now have integration and unit tests for Avro schemas functionality.

@libretto libretto force-pushed the avro_references2 branch 2 times, most recently from f0f9450 to 0167a75 Compare July 29, 2024 09:46
@libretto libretto force-pushed the avro_references2 branch 3 times, most recently from 9cac0ba to f4346ea Compare September 26, 2024 16:57
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.

4 participants