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

Improve tests for Kafka topic create and update #361

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

ivanyu
Copy link
Member

@ivanyu ivanyu commented Sep 12, 2023

No description provided.

@ivanyu ivanyu marked this pull request as ready for review September 12, 2023 13:46
@ivanyu ivanyu requested review from a team as code owners September 12, 2023 13:46
@ivanyu ivanyu requested a review from heikju September 12, 2023 13:46
Copy link
Contributor

@eliax1996 eliax1996 left a comment

Choose a reason for hiding this comment

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

Syntattically it's fine, I'm asking a couple of question to understand also the semantics.

Are we executing similar tests in an end to end scenario against a real system?

Because they seems like a contract testing about the shape of the expected data. But, if we don't test that the provider is compliant with this specification, both the systems can succeed with the test suite but in reality they are talking a different language when deployed together they can fail.

Also here we have a dynamicity in the shape inside the test. We are not enforcing that the reply is of a certain shape but we are only enforcing that is containing exactly the same data that we have given to the other component, maybe we want also to have a set of keys that we expect always to have?

tests/test_cli.py Show resolved Hide resolved
tests/test_cli.py Show resolved Hide resolved
@ivanyu
Copy link
Member Author

ivanyu commented Sep 12, 2023

Are we executing similar tests in an end to end scenario against a real system?

Because they seems like a contract testing about the shape of the expected data. But, if we don't test that the provider is compliant with this specification, both the systems can succeed with the test suite but in reality they are talking a different language when deployed together they can fail.

No, we don't have end-to-end tests for this client (except for some internal Aiven testing). Most tests in test_cli.py don't test much beyond the CLI parsing. What I added are indeed contract tests: we "know" the Aiven API is correct, so we're testing the client is compliant with its schema.

@ivanyu
Copy link
Member Author

ivanyu commented Sep 12, 2023

Also here we have a dynamicity in the shape inside the test. We are not enforcing that the reply is of a certain shape but we are only enforcing that is containing exactly the same data that we have given to the other component, maybe we want also to have a set of keys that we expect always to have?

I don't understand this. Could you elaborate?

@eliax1996
Copy link
Contributor

No, we don't have end-to-end tests for this client (except for some internal Aiven testing). Most tests in test_cli.py don't test much beyond the CLI parsing. What I added are indeed contract tests: we "know" the Aiven API is correct, so we're testing the client is compliant with its schema.

Now I got everything, sorry for not understanding it initially!
LGTM

@eliax1996 eliax1996 merged commit 9947d2e into main Sep 12, 2023
25 checks passed
@eliax1996 eliax1996 deleted the ivanyu/kafka-topic-tests branch September 12, 2023 15:31
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