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

fix: handle InMemoryDatabase find_schemas latest when no versions available for subject #835

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

biggusdonzus
Copy link
Contributor

@biggusdonzus biggusdonzus commented Mar 7, 2024

This PR fixes an IndexError: list index out of range exception raised when calling InMemoryDatabase.find_schemas with argument latest_only = True and there's a schema with no versions.

@biggusdonzus biggusdonzus force-pushed the fdorlandi-sentry-list-index-out-of-range branch 2 times, most recently from 8b0e307 to c382270 Compare March 7, 2024 23:31
@biggusdonzus biggusdonzus requested a review from aiven-anton March 8, 2024 08:17
@biggusdonzus biggusdonzus marked this pull request as ready for review March 8, 2024 08:17
@biggusdonzus biggusdonzus requested review from a team as code owners March 8, 2024 08:17
from karapace.in_memory_database import InMemoryDatabase, Subject


def test_find_schemas() -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: As this test has a very specific purpose: verify we don't blow up when given an empty string, I think we should have more specific naming of this test. I find this to be the best way to structure test for maximizing communication of original intent:

class TestFindSchemas:  # <- _what_ are we testing
    def test_returns_empty_list_when_no_schemas(  # <- what _expectation_ do we have on the tested thing
        self,
    ) -> None:
        ... 
    # Future tests can easily be added here:
    def test_raises_foo_error_when_stars_are_not_aligned() -> None: ...

Copy link
Contributor

@aiven-anton aiven-anton left a comment

Choose a reason for hiding this comment

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

Thanks, this is great. I could merge this as-is, but would suggest adding some more context to the test, such that future readers understand your intent.

@biggusdonzus biggusdonzus force-pushed the fdorlandi-sentry-list-index-out-of-range branch from c382270 to 8f6562a Compare March 8, 2024 10:26
@aiven-anton aiven-anton merged commit ea3300d into main Mar 8, 2024
8 checks passed
@aiven-anton aiven-anton deleted the fdorlandi-sentry-list-index-out-of-range branch March 8, 2024 10:59
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.

2 participants