RFR: 1589: Improve TestIssue and TestPullRequest [v2]

[Magnus Ihse Bursie]

On Thu, 8 Sep 2022 20:13:58 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 incrementally with one additional commit since the last revision:
> 
>   Unused imports

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java line 233:

> 231:             // Check that the review is flagged as stale
> 232:             TestBotRunner.runPeriodicItems(checkBot);
> 233:             authorPr = author.pullRequest(authorPr.id());

What was this line even supposed to accomplish? Some kind of reload?

Edit: I think I get it now.

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

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