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 > > > > >
