[GitHub] [geode] demery-pivotal opened a new pull request #7210: GEODE-9897: Improve ClientUserAuthsTest

[GitBox <gi...@apache.org>]

demery-pivotal opened a new pull request #7210:
URL: https://github.com/apache/geode/pull/7210


   PROBLEM 1
   
   In `ClientUserAuthsTest`, the `before()` method uses the `spy()`
   mechanism to override `getNextId()` on the `ClientUserAuths` instance it
   is testing. This bypasses the actual implementation for all tests,
   leaving the following important `ClientUserAuths` responsibilities
   untested:
   - Detect when all IDs have all been used.
   - Clear all existing authorizations when all IDs have been used, to
     force reauthentication.
   - Re-seed the ID generator when all IDs have been used.
   
   The tests are likely doing this because `getNextId()` relies on a
   `Random`, which is uncontrollable by design. Because `ClientUserAuths`
   creates the `Random`, the tests are unable to inject a testable
   instance, and so instead must bypass the `ClientUserAuths` methods that
   interact with the `Random`.
   
   PROBLEM 2
   
   Some tests attempt to verify `ClientUserAuths` responsibilities only by
   observing how a `ClientUserAuths` interacts with itself. These tests
   verify only implementation details, and leave the actual responsibility
   untested.
   
   PROBLEM 3
   
   If the `id` passed to `ClientUserAuths.putSubject(subject, id)` is 0 or
   -1, the method treats that as a request to to generate and assign an ID.
   The `getNextID()` method can return either of these special values, and
   so `putSubject()` might associate the subject with one of them. If a
   caller later attempts to replace that subject by calling
   `putSubject(replacement, id)` with the same ID (0 or -1), `putSubject()`
   will generate a new ID and assign it to the replacement, rather than
   replacing the original subject.
   
   SOLUTIONS
   
   1. Extract and test a `RandomSubjectIdGenerator` that generates random
      IDs.
   2. Change existing tests so that they do not override production
      behavior, and so that they verify actual responsibilities.
   3. Update `getNextID()` to avoid returning 0 or -1.
   
   A subsequent PR can add tests for the now-testable responsibilities.
   


-- 
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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] demery-pivotal closed pull request #7210: GEODE-9897: Improve ClientUserAuthsTest

[GitBox <gi...@apache.org>]

demery-pivotal closed pull request #7210:
URL: https://github.com/apache/geode/pull/7210


   


-- 
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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org