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

Write zio-kafka-testkit documentation #846

Merged
merged 3 commits into from
May 21, 2023
Merged

Write zio-kafka-testkit documentation #846

merged 3 commits into from
May 21, 2023

Conversation

guizmaii
Copy link
Member

@guizmaii guizmaii commented May 12, 2023

Related to #845

The doc can be read here: https://github.com/zio/zio-kafka/blob/testing_doc/docs/writing-tests.md

TODO:

  • Add ZIOSpecWithKafka doc
  • use mdoc

@guizmaii guizmaii force-pushed the testing_doc branch 3 times, most recently from ae20ca3 to 8f775d0 Compare May 12, 2023 15:58
docs/writing-tests.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@erikvanoosten erikvanoosten left a comment

Choose a reason for hiding this comment

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

Nice! We really need something like this.
Would you like me to pick up a part of the work? For example, I could write some examples for testing zio-kafka consumers.

docs/writing-tests.md Outdated Show resolved Hide resolved
docs/writing-tests.md Show resolved Hide resolved
```

Let's study some simple examples of tests you can write with the `zio-kafka-testkit` and `zio-test`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two important techniques for testing zio-kafka apps. One is to produce test data so that we can test a consumer, two is to consume from a topic so that we can test a producer. I would love to see that described in the introduction here.

docs/writing-tests.md Outdated Show resolved Hide resolved
docs/writing-tests.md Show resolved Hide resolved
@guizmaii guizmaii force-pushed the testing_doc branch 2 times, most recently from 89cb239 to 61c8b6a Compare May 14, 2023 16:26
val producer: ZLayer[Kafka, Throwable, Producer] =
(ZLayer.fromZIO(producerSettings) ++ ZLayer.succeed(Serde.string: Serializer[Any, String])) >>>
Copy link
Member Author

Choose a reason for hiding this comment

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

This ZLayer.succeed(Serde.string: Serializer[Any, String]) was useless

Comment on lines -48 to -49
(ZLayer.fromZIO(transactionalProducerSettings) ++ ZLayer.succeed(
Serde.string: Serializer[Any, String]
)) >>>
Copy link
Member Author

Choose a reason for hiding this comment

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

This ZLayer.succeed(Serde.string: Serializer[Any, String]) was useless

@guizmaii guizmaii force-pushed the testing_doc branch 3 times, most recently from c8a80ff to d319dc2 Compare May 14, 2023 17:37
@guizmaii guizmaii marked this pull request as ready for review May 14, 2023 18:01
@guizmaii guizmaii force-pushed the testing_doc branch 3 times, most recently from c0397a4 to 99b3989 Compare May 14, 2023 18:22
@guizmaii guizmaii force-pushed the testing_doc branch 7 times, most recently from f8a2597 to 8e9ece8 Compare May 14, 2023 18:53
@guizmaii guizmaii force-pushed the testing_doc branch 3 times, most recently from 1bc7ae4 to 3b08395 Compare May 20, 2023 06:24
Copy link
Collaborator

@erikvanoosten erikvanoosten left a comment

Choose a reason for hiding this comment

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

Nice!
Some small remarks but nothing exciting.

Comment on lines +86 to +87
The `sequential` aspect from zio-test is used to specify that the tests in the suite must be run sequentially. This is necessary because Kafka is a shared resource.
We don't want tests to interfere with each other.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible to write your tests such that they do not interfere with each other. Therefore I propose to weaken the text a bit to:

Suggested change
The `sequential` aspect from zio-test is used to specify that the tests in the suite must be run sequentially. This is necessary because Kafka is a shared resource.
We don't want tests to interfere with each other.
The `sequential` aspect from zio-test is used to specify that the tests in the suite must be run sequentially instead of concurrently. Kafka is shared between the tests so running them sequentially prevents tests to influence each other.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're saying the same thing but differently

Copy link
Collaborator

Choose a reason for hiding this comment

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

The original text uses the word 'necessary'. This is strictly not correct because there are other ways to achieve the same goal.

Comment on lines +84 to +85
Finally, we annotate the suite with the `timeout` and `sequential` aspects.
The `timeout` aspect from zio-test is used to specify a timeout for the entire suite. If the suite takes more than 5 minutes to run, it will fail.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose that we split the paragraph in 2 (add a newline after it will fail):

Suggested change
Finally, we annotate the suite with the `timeout` and `sequential` aspects.
The `timeout` aspect from zio-test is used to specify a timeout for the entire suite. If the suite takes more than 5 minutes to run, it will fail.
Finally, we annotate the suite with the `timeout` and `sequential` aspects.
The `timeout` aspect from zio-test is used to specify a timeout for the entire suite. If the suite takes more than 5 minutes to run, it will fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to keep these things grouped.

The paragraph is structured in 3 parts:

  1. Introduction to the concept we'll study in this paragraph: "Finally, we annotate the suite with the timeout and sequential aspects."
  2. The first concept: "The timeout aspect from zio-test ..."
  3. The second concept: "The sequential aspect from zio-test ..."

docs/writing-tests.md Show resolved Hide resolved

Finally, we use the `KafkaRandom` trait from zio-kafka-testkit and its methods to generate random values for the Consumer client ID, the Consumer group ID and the topic name.
More details about this `KafkaRandom` trait [later in this page](#kafkarandom-trait).
Using random values for these parameters is important to avoid conflicts between tests as we share one Kafka instance between all the tests of the suite.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Saying we share a Kafka instance is enough, how is less important for this paragraph.

Also, as I have said before, I am not in favor of using random values. I prefer my test parameters to be the same for each run. (With a big exception for property based testing.)

Also, since this text is important, IMHO it should be a new paragraph.

Suggested change
Using random values for these parameters is important to avoid conflicts between tests as we share one Kafka instance between all the tests of the suite.
Using different values for these parameters is important to avoid conflicts between tests as we share one Kafka instance between the tests. Using random values is a convenient way to make sure they are different.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to keep as is

Copy link
Collaborator

Choose a reason for hiding this comment

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

That answer disappoints me a bit. I'd have preferred you tell me where I am wrong.

docs/writing-tests.md Show resolved Hide resolved
Base automatically changed from rename_testkit_module to master May 21, 2023 17:44
…ke it more explicit that it's something that can be used by zio-kafka users

Also, change the base namespace of this module from `zio.kafka` to `zio.kafka.testkit` to make things more explicit
Co-authored-by: Erik van Oosten <e.vanoosten@grons.nl>
@guizmaii guizmaii merged commit bf336fc into master May 21, 2023
4 of 11 checks passed
@guizmaii guizmaii deleted the testing_doc branch May 21, 2023 18:11
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.

None yet

3 participants