[jira] [Commented] (GEODE-9897) ClientUserAuthsTest leaves some seemingly tested responsibilities untested

["ASF subversion and git services (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-9897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17476471#comment-17476471 ] 

ASF subversion and git services commented on GEODE-9897:
--------------------------------------------------------

Commit a5644c313010ccbb3cb6649e66fad80278f12c0d in geode's branch refs/heads/develop from Dale Emery
[ https://gitbox.apache.org/repos/asf?p=geode.git;h=a5644c3 ]

GEODE-9897: Improve ClientUserAuthsTest (#7263)

* GEODE-9897: Improve ClientUserAuthsTest

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.

* Add tests for handling ID exhaustion

> ClientUserAuthsTest leaves some seemingly tested responsibilities untested
> --------------------------------------------------------------------------
>
>                 Key: GEODE-9897
>                 URL: https://issues.apache.org/jira/browse/GEODE-9897
>             Project: Geode
>          Issue Type: Test
>          Components: tests
>            Reporter: Dale Emery
>            Assignee: Dale Emery
>            Priority: Major
>              Labels: GeodeOperationAPI, pull-request-available
>
> *Problem 1:* The test class's {{before()}} method uses the {{spy()}} mechanism to override {{getNextId()}} on the {{ClientUserAuths}} it is testing. This bypasses the actual implementation for all tests, leaving untested the following important {{ClientUserAuths}} responsibilities:
>  - Detect when all IDs have all been used.
>  - Clear all existing authentications 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{}}}.
> The solution to this problem is to extract and test an ID generator that accepts a {{Random}} as a constructor parameter, thus allowing tests to control it.
> This also frees {{ClientUserAuths}} from the responsibility to generate IDs, so it can instead focus on how it uses the IDs. Then it will be straightforward to write tests to verify that {{ClientUserAuths}} forces reauthentication when the generator runs out of IDs.
> *Problem 2:* Several tests in this class attempt to verify certain responsibilities only by spying on how the {{ClientUserAuths}} interacts with itself. Verifying only these internal interactions leaves the actual responsibilities (how the {{ClientUserAuths}} responds to subsequent calls) untested.
> The tests can instead verify the actual responsibilities by calling public methods and making assertions about the returned values.



--
This message was sent by Atlassian Jira
(v8.20.1#820001)