lymerin commented on PR #7625: URL: https://github.com/apache/shenyu/pull/7625#issuecomment-6188599842
Thanks @Aias00 for the detailed [review](https://github.com/apache/shenyu/pull/7625#pullrequestreview-5527403629) and for checking the identity changes. I investigated the CI failure and pushed the fix in `6a29ba5e1`. 1. **Failing test, root cause, and why the initial validation missed it** > the new test fails in CI The failure came from an unclosed static mock in the existing `ApacheDubboGrayLoadBalanceTest`. Its mock of `LoadBalancerFactory.selector()` remained active on the test thread and affected the subsequent `ApacheDubboPluginDataHandlerTest`. Our initial targeted validation passed 103 tests, but it did not reproduce the execution order that exposed this leak: the existing mock-based test running before the new test in a shared JVM. That gap in our validation is why we did not catch it before the first submission. I reproduced the failure with that order. Running the same tests in separate JVMs passed, confirming the test isolation issue. I then added `@AfterEach` cleanup to close the static mock. With the fix, all 53 tests in the Apache Dubbo module passed in the order that reproduced the failure. I also added a comment explaining the cleanup. 2. **Constant selector/rule attachments and the selection sequence** > identical inputs cannot yield a varying sequence The selector/rule attachments identify the configuration to use. Within that configuration, weighted round robin maintains internal state and advances it on each selection. Therefore, the same attachments produce the asserted sequence across repeated calls. I added a comment next to the assertion to make this behavior explicit. 3. **The two suggested test changes** > Fix the test The current gray-routing implementation reads group/version from the selected upstream; it does not read per-call group/version attachments. I retained the sequence assertion because it verifies that the three same-address variants are actually selected according to their individual weights. The preceding cache assertions verify that the entries are preserved, but do not verify the selection behavior. Both the existing and new assertions remain intact. 4. **Protocol immutability** > there is no `setProtocol` Agreed: protocol has no setter and is assigned during construction. No additional invalidation is needed for it. Thanks again for catching the failure. The update fixes the test isolation issue without changing production routing behavior. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
