Skip to content

Add some more tests#28

Open
caldempsey wants to merge 2 commits into
svix:mainfrom
caldempsey:callum/negative-tests
Open

Add some more tests#28
caldempsey wants to merge 2 commits into
svix:mainfrom
caldempsey:callum/negative-tests

Conversation

@caldempsey

Copy link
Copy Markdown

Adds a few negative tests. These two are suspect:

def test_to_from_base62_invalid_base62_string():
    # Arrange
    ksuid = Ksuid()
    invalid_base62 = "invalid_base62_string!"

    # Act & Assert
    with pytest.raises(ValueError):
        ksuid.from_base62(invalid_base62)

def test_to_from_base62_empty_string():
    # Arrange
    ksuid = Ksuid()
    empty_base62 = ""

    # Act & Assert
    with pytest.raises(IndexError): # this looks wrong
        ksuid.from_base62(empty_base62)

@svix-jbrown svix-jbrown force-pushed the callum/negative-tests branch from 94c09a5 to 86ab131 Compare May 5, 2026 17:14
@svix-jbrown svix-jbrown requested a review from a team as a code owner May 5, 2026 17:14
@svix-jplatte

Copy link
Copy Markdown
Member

Trying to get CI to run..

@svix-jplatte svix-jplatte changed the title feat: Negative Tests Add some more tests Jun 22, 2026
@svix-jplatte svix-jplatte enabled auto-merge (squash) June 22, 2026 11:09
@svix-jplatte svix-jplatte disabled auto-merge June 22, 2026 11:09
@svix-jplatte

Copy link
Copy Markdown
Member

Wait, I'm a bit confused. Did these pass originally? The IndexError one at least looks like it's meant to be mergable and document the status quo so any changes to that behavior would have a test that would confirm the intended change.

@svix-jbrown

Copy link
Copy Markdown
Contributor

Wait, I'm a bit confused. Did these pass originally? The IndexError one at least looks like it's meant to be mergable and document the status quo so any changes to that behavior would have a test that would confirm the intended change.

We made some big changes to this library since this PR was submitted 2 years ago...

@caldempsey

Copy link
Copy Markdown
Author

Don't believe they passed originally no.

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