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

Avro references2 #32

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Avro references2 #32

wants to merge 8 commits into from

Conversation

libretto
Copy link
Collaborator

About this change - What it does

References: #xxxxx

Why this way

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 256, 265-269
  schema_reader.py 558-559, 561-566
  schema_registry_apis.py 1059
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.

3 participants