#2320: Unit Tests for ACL Credential Propagation in JedisPool/UnifiedJedis #4121
Conversation
- Unit test for constructors of JedisPool/UnifeidJedis which verifies that the provided username/password reaches ConnectionFactory. Without the need of explicit test env
|
Hi @YoHanKi, With commit we have introduced dedicated test user acljedis different than default one
and added integration tests In the comment #2320 (comment) it is mentioned that there are still missing tests, what I meant is to try to identify them and add any missing ones. Optionally we can create a unit test for JedisPoolUnitTest, ..., which exercises JedisPool object constructions.
Another option instead of using Reflection is to use Mocking object construction Hope it helps... |
- Unit test for constructors of JedisPool/UnifeidJedis which verifies that the provided username/password reaches ConnectionFactory. Without the need of explicit test env
- Unit test for constructors of JedisPool/UnifeidJedis which verifies that the provided username/password reaches JedisFactory/ConnectionFactory. Without the need of explicit test env
- Unit test for constructors of JedisPool/UnifeidJedis which verifies that the provided username/password reaches JedisFactory/ConnectionFactory. Without the need of explicit test env - In addition to the existing integration tests that connect with the dedicated non-default user "acljedis"
|
Hello @ggivo , Thank you for your guidance. I have written tests based on your feedback and did my best, though I feel my skills are still quite limited. I really appreciate your support for a junior developer like me. |
|
@YoHanKi |
…magic numbers (redis#2320) - Added unit tests for JedisPool/UnifiedJedis constructors to verify that the provided username/password reaches JedisFactory/ConnectionFactory - Change verification method to find necessary fields instead of using magic numbers
|
Hello, @ggivo Thank you very much for your kind attention and review. Before you looked into JedisPoolUnitTest, I was concerned that the previous usage of magic numbers (such as context.arguments().get(1)) might lead to potential issues, so I modified it accordingly. If you feel that the previous approach is more suitable, I would be more than happy to revert the change. Once again, I sincerely appreciate your thoughtful review and support. |
Questions
In your previous feedback, you mentioned, "I would start with reviewing submitted tests and check if there are other places where we configure username/password with missing tests." Does this mean that username/password should be passed in every test, or are you referring to specific tests (e.g., JedisPoolTest or JedisPooledTest)?
Regarding the approach of using a test-specific subclass (TestJedisFactory) to verify, without using Reflection, that the ACL credentials provided to the constructor are correctly reflected in the internal JedisClientConfig—is this approach acceptable? Are there any additional aspects we should check?
Additionally, we would like to confirm if the placement of these tests is appropriate.