membphis commented on PR #13974: URL: https://github.com/apache/apisix/pull/13974#issuecomment-6628205034
The separator change works, but the new regression test does not keep the logger mock active for the request that should trigger the error. At `2430bbf20b1bbd349b3e3655e8d99c842731d75a`, TEST 22 installs the mock in its `extra_init_by_lua` block and only creates the route. TEST 23 sends `GET /hello` and checks the error log, but it does not include that hook. The hook is not inherited between test blocks: the NGINX configuration for TEST 23 restores the real socket logger, so it does not produce `mocked send failure`. I applied the patch to `master@79a87e559f6bec9a018ca4466e31b43ef9650e81` and ran the added TEST 22/23 blocks in isolation using the repository's Test::Nginx harness and APISIX runtime 1.4.2: | Test setup | Baseline | With this PR applied | | --- | --- | --- | | Added TEST 22/23 unchanged | 5/6 assertions passed | 5/6 assertions passed | | Same mock also installed in TEST 23 | 5/6 assertions passed | 6/6 assertions passed | The failing assertion in the original tests is the expected `failed to log message: mocked send failure` error line. A fresh rerun reproduced these results. Keeping the mock active demonstrates that the production string fix is effective, while the submitted regression test still fails. Please install the mock in the block that actually sends the request, or combine route creation, the request, and the assertion into one block with the mock active. -- 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]
