RFR: 1589: Improve TestIssue and TestPullRequest [v3]

[Erik Joelsson]

On Fri, 9 Sep 2022 17:35:32 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> While working on [SKARA-1565](https://bugs.openjdk.org/browse/SKARA-1565) I've realized that the current test implementations of `Issue` and `PullRequest` (TestIssue and TestPullRequest) do not represent the behavior of their real implementation counterparts very well. For [SKARA-1565](https://bugs.openjdk.org/browse/SKARA-1565), this discrepancy in behavior makes it impossible to write new tests for the poller functionality and at the same time keeping existing tests for any specific bot working when switching the bot to using the poller. Fixing the tests for this turned out to be a pretty large change, so I'm moving it to a separate bug to make reviews easier.
>> 
>> The main issue here is the difference in lifecycle between instances of the test classes and the real classes. In production, when querying an `IssueProject` for an Issue, what you get is basically an object instance which contains a snapshot of the external server side data for the issue. When you do another query, you get a new instance with potentially more up to date data. Some data is not included in the snapshot and will be dynamically fetched from the remote server on request, e.g. comments. Any method on the production implementation of Issue that adds or modifies data calls through to the external server and does not update the current snapshot.
>> 
>> In contrast to the above behavior, the `TestIssue`, when returned from the `TestIssueProject`, will be a new instance each time, but that instance shares most of the underlying data directly with all other instances of the "same" issue. So most additions or updates are immediately reflected in all instances of the "same" issue. For the poller tests, this makes it impossible to simulate having different snapshots returned at different times, and comparing the contents of them to figure out if something has changed. I'm also worried that we risk inadvertently building in assumptions in our bot code that relies on this behavior.
>> 
>> I'm fixing this by changing `TestIssue` and `TestPullRequest` to better mimic the behavior of the production implementations. To achieve this, I'm introducing two (new) classes TestIssueStore and `TestPullRequestStore`. These classes represent the server side state of these objects. The `TestIssue` and `TestPullRequest` are changed to take similar snapshots as the production classes, and any method that modifies the state will just call through and modify the `*Store` instance. For test code to have an easier time inspecting "server side" data, the `*Store` objects are exposed to test code.
>> 
>> The bulk of the changes are updates to existing test code, mostly to use the new `TestPullRequest::store` method to verify state in a pull request. Another notable change is that `HostCredentials` class (which is where test code generally create new `PullRequest` and `Issue` instances, now returns `TestPullRequest` and `TestIssue` which makes it easy for tests to get hold of the backing `*Store` instances. I'm also removing all instances of this construct:
>> 
>> 
>>             // Make sure that the push registered
>>             var lastHeadHash = pr.headHash();
>>             var refreshCount = 0;
>>             do {
>>                 pr = author.pullRequest(pr.id());
>>                 if (refreshCount++ > 100) {
>>                     fail("The PR did not update after the new push");
>>                 }
>>             } while (pr.headHash().equals(lastHeadHash));
>> 
>> I don't think it was needed before this patch, and I'm fairly certain it's not needed now. The `headHash` field of a TestPullRequest is fetched from the underlying repository on instance creation. 
>> 
>> This is a test only change not touching any product code.
>
> Erik Joelsson has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Missed correcting a test causing compiler error
>  - Merge branch 'master' into SKARA-1589-test-refactor
>  - Unused imports
>  - More test fixes
>  - Test refactor

I seem to have an intermittent failure in BackportsTest on Windows. Still trying to figure it out, so will hold off on this for now.

-------------

PR: https://git.openjdk.org/skara/pull/1370