-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[improve][pip] PIP-390 Improve the reusability of Pulsar test code and best practice for unit and integration tests for Pulsar #23565
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
Open
heesung-sn
wants to merge
2
commits into
apache:master
Choose a base branch
from
heesung-sn:heesung-pip
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+79
−0
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
# PIP-390: Improve the reusability of Pulsar test code and best practices for unit and integration tests in Pulsar | ||
|
||
## Background | ||
|
||
- Pulsar integration tests are located in the `tests` module, which run on [testcontainers](https://github.com/testcontainers). | ||
- Pulsar unit tests are located in each module under the test scope, with many tests running on mocked (in-memory) Pulsar clusters. | ||
- Currently, Pulsar tests run on the `testng` framework. | ||
|
||
## Motivation | ||
|
||
Apache Pulsar has several reusable test code, such as client test cases, container setup, and other helper functions in the `tests` module. However, since this module is only accessible within the "test" scope, it is difficult to reuse these utilities in other projects. | ||
|
||
The Pulsar community has discussed the ideal practice for unit and integration tests (see discussion [here](https://lists.apache.org/thread/fn3rk1x7v9291qh3g6vf4jxhvq6zc4mm)). This PIP aims to document the results of the discussion for future reference. | ||
|
||
|
||
## Goals | ||
|
||
### In Scope | ||
|
||
1. Improve the reusability of Pulsar test code, especially for the `tests` module. | ||
2. Document best practices for unit and integration tests in Pulsar. | ||
|
||
### Out of Scope | ||
|
||
- Propose the next generation of the test framework and improvement plan(This can be a separate pip). | ||
|
||
## High-Level Design | ||
|
||
### 1. Improve the Reusability of Pulsar Test Code in the `tests` Module | ||
- Decouple client test logic from cluster setup and test runner code, allowing the test logic to be maintained independently of test environment and test runner framework changes. | ||
|
||
## Detailed Design | ||
|
||
### 1. Improve the Reusability of Pulsar Test Code in the `tests` Module | ||
|
||
Refactor the `tests` module as follows: | ||
|
||
- `tests`: Contains the main integration test runners built under the "test" scope (e.g., test runner classes using TestNG, dependent on `pulsar-inttest-lib`, `pulsar-inttest-client`, and other modules). | ||
- `pulsar-inttest-lib`: Contains common libraries for integration tests (e.g., test utilities, test containers, and other reusable test setups). | ||
- `pulsar-inttest-client`: Contains common client-side integration test cases (should include only client-side test logic and assertions, depending on `PulsarClient` and `PulsarAdmin`). These test cases can be reused in `tests` and other modules. | ||
|
||
### 2. Document Best Practices for Unit and Integration Tests in Pulsar | ||
|
||
Based on the [community discussion](https://lists.apache.org/thread/fn3rk1x7v9291qh3g6vf4jxhvq6zc4mm), Pulsar contributors are expected to consider the following guidelines when adding unit and integration tests: | ||
|
||
- **Reuse Test Classes**: When possible, add new test cases as `@Test` annotations within existing test classes instead of creating new classes, as additional classes can increase test overhead. | ||
|
||
- **Unit Tests Per Class**: Contributors should add unit tests for each new class they introduce, focusing on the behavior of the class’s public methods. Deep state checks using reflection tools should be avoided, as they may break with future implementation changes. Reference: [Testing on the Toilet - Test Behavior, Not Implementation](https://testing.googleblog.com/2013/08/testing-on-toilet-test-behavior-not.html?m=1). | ||
If testing public methods in specific class states is necessary, a constructor with the `@VisibleForTesting` annotation can be added to allow injection of (mocked) member variables. | ||
|
||
- **Use In-Memory Test Cluster**: For tests requiring a Pulsar cluster, consider using an in-memory test cluster (e.g., by extending `MockedPulsarServiceBaseTest` or `PulsarTestContext`). This approach is also useful for "mocking" Pulsar behavior. | ||
|
||
- **Use Container Test Cluster for Integration Tests**: When the test can run solely with Pulsar client logic and no Pulsar behavior mocking is needed, consider adding it as an integration test using a container-based test cluster. However, container-based tests are heavier than in-memory tests, as they require a containerized cluster setup. Integration tests are typically reserved for core features and Pulsar sink/source/function IO tests. | ||
|
||
- **Add Shared Client Logic to `pulsar-inttest-client`**: Include/move common test client logic here if it can be reused. [AssertionJ](https://github.com/assertj/assertj) can be used for assertion statements, which is compatible with both Junit and TestNg frameworks. | ||
- **Add Shared Libraries to `pulsar-inttest-lib`**: Place reusable test setups or test utilities in this module. | ||
|
||
## Public-Facing Changes | ||
|
||
### Configuration | ||
|
||
- N/A | ||
|
||
### API Changes | ||
|
||
- N/A | ||
|
||
## Backward & Forward Compatibility | ||
|
||
These changes apply only to test classes, so they do not impact backward or forward compatibility. | ||
|
||
## Security Considerations | ||
|
||
- N/A | ||
|
||
## Links | ||
|
||
- [Initial mailing list discussion thread](https://lists.apache.org/thread/fn3rk1x7v9291qh3g6vf4jxhvq6zc4mm) | ||
- Mailing list PIP voting thread (to be added) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that directly providing the framework link may be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is Pulsar's testing going to switch to using JUnit 5 and remove testng?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@falser101 Perhaps eventually, but that will take a very long time. Instead of having the goal of converting from TestNG to JUnit5, I believe that it would be more valuable to first provide ways to create JUnit5 tests. There's not much value in doing a 1-to-1 mapping from TestNG to JUnit5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, maybe we can commit the first JUnit5 test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized this pip is too much ambitious, and I am not sure it is practical any more. I am thinking of closing this PIP.