Hi, I agree with your points that there are tradeoffs between in-memory vs container test clusters, and that we should add more "unit" tests to cover behavior and avoid deep state checks. We should smartly move/add integ tests in in-memory or container test clusters, without increasing the test execution time.
Also, I think there are some rooms to optimize our integ tests. For example, many of the integ tests can run in parallel, under the same cluster. We probably need to add test cases more often instead of creating new test classes. Also, we should reuse PulsarClient and PulsarAdmin as much as possible, as initiating them in each test can add several seconds more. Some of the test classes can be merged, and the clusters can be shared among them too. I consider my proposal received positive feedback in this thread, so I will move on and raise a PR for this change. Thanks, Heesung On Fri, Nov 1, 2024 at 2:06 PM Lari Hotari <lhot...@apache.org> wrote: > > I guess the problem is that most Pulsar tests are integration tests and there > aren't many actual unit tests for the most important parts of the Pulsar code > base. > > There's also a lot of tests which test the implementation and not publicly > observable behaviors that is typically a recommendation. For example testing > private or protected methods is a "code smell" hinting that a new concept or > a "unit" should be extracted which has it's own API that is tested in a unit > test. We don't consistently follow this guideline in the Pulsar code base so > there's a lot of bad examples around. We can improve over time. > Reference: > https://testing.googleblog.com/2013/08/testing-on-toilet-test-behavior-not.html?m=1 > > One could argue that it's a problem in modularity of the code base when > testing is hard or slow. Defaulting to integration tests is a tradeoff. The > benefit is that integration is handled well, but the downside is that it > leads to tightly coupled code bases. It's way too easy to have any module > access anything it needs without thinking much about modularity. > > -Lari > > On 2024/11/01 03:05:33 Heesung Sohn wrote: > > Hi, > > > > > if we're not going to mock some behaviors of ZK or BK, it might not be > > > good. > > I agree. I think we can put/move more tests in the integration tests, > > especially for those tests that are based on client and admin calls > > only. > > > > Also, we need to think about how to optimize the integration test runs. > > For example, we probably need to reuse the container clusters as much > > as possible. (I guess this is a separate topic, though.) > > > > > > Thanks, > > Heesung > > > > > > On Thu, Oct 31, 2024 at 7:06 PM Yunze Xu <x...@apache.org> wrote: > > > > > > This is a good start to. I have another suggestion. Currently we > > > relied heavily on mock ZK and BK services. However, if we're not going > > > to mock some behaviors of ZK or BK, it might not be good. I have > > > encountered the following error logs in the downstream application: > > > > > > ``` > > > > > > 12:28:36.903 > > > [ZKMetadataStore-216-1:org.apache.pulsar.broker.PulsarService@1167] > > > INFO org.apache.pulsar.broker.PulsarService - This broker was elected > > > leader > > > ... (400+ lines) > > > 12:28:44.630 > > > [ZKMetadataStore-12-1:org.apache.pulsar.broker.PulsarService@1167] > > > INFO org.apache.pulsar.broker.PulsarService - This broker was elected > > > leader > > > ``` > > > > > > However, eventually I found it's an issue with mock ZK or BK. After > > > changing the BK to `LocalBookkeeperEnsemble`, the errors above > > > disappeared. We should really avoid mocking external services as much > > > as possible. > > > > > > Thanks, > > > Yunze > > > > > > On Fri, Nov 1, 2024 at 9:55 AM Jie crossover <crossover...@gmail.com> > > > wrote: > > > > > > > > LGTM, the current test code is relatively confusing, and this proposal > > > > can > > > > improve this situation. > > > > -- > > > > Best Regards！ > > > > crossoverJie > > > > > > > > > > > > Heesung Sohn <hees...@apache.org> 于2024年11月1日周五 06:16写道： > > > > > > > > > Hi all, > > > > > > > > > > I think we can refactor/split the `tests` module(the integration test > > > > > module) to reuse the integration test code better. > > > > > > > > > > Currently, there are good test classes to reuse(such as topic message > > > > > test cases, container setup, and other test util funcs), but this > > > > > module is built under "test" scope only, making it hard to reuse in > > > > > other projects. > > > > > > > > > > I propose the following module structure. > > > > > > > > > > `tests` : contains actual integ test runners built under test > > > > > scope(e.g. test-ng test classes, depending on `pulsar-inttest-lib, > > > > > `pulsar-inttest-client` and other modules) > > > > > > > > > > `pulsar-inttest-lib` : contains common test libraries(test-utils, test > > > > > containers, and other sharable integ test setups) > > > > > > > > > > `pulsar-inttest-client` : contains common integ-test client cases (it > > > > > should contain only client side test logic, depending on PulsarClient > > > > > and PulsarAdmin). These test cases can be used in `tests`. > > > > > > > > > > I am looking forward to your feedback. > > > > > > > > > > Thanks, > > > > > Heesung > > > > > > >
