RFR: 1589: Improve TestIssue and TestPullRequest [v2]

[Erik Joelsson]

On Fri, 9 Sep 2022 16:43:59 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> issuetracker/src/test/java/org/openjdk/skara/issuetracker/IssueTrackerTests.java line 130:
>> 
>>> 128:             var links = issue2.links();
>>> 129:             assertEquals(1, links.size());
>>> 130:             assertEquals(link.relationship(), links.get(0).relationship());
>> 
>> Here's an obvious drawback of the "var" syntax. What type is this `links`? Why did not an ordinary equals suffice? Does this really relate to the rest of the changes, or is it an independent fix that happened to tag along?
>
> Yes, in modern intellij, I get grey type names for every `var` variable, which makes it really convenient. I too have to open most PRs I review in the IDE to properly read the code.
> 
> Regarding why equals won't suffice. Before this patch, `TestIssue::links` would just return a list of all `Link` objects that had been added to the Issue. In this patch, I had to change it to always refresh the target `Issue` of each link with a fresh snapshot, because that's what happens when you query for any real links. And since Link is read-only that means creating a new `Link` instance each time as well. Another way to solve this would be to implement `Link::equals` to take this into account. That would probably be a good thing to do, but then this patch will touch product code. Long term, that's probably better and I can do that if you want.

Looking closer at this, the `Link::equals` already does the right thing. The reason it's not sufficient is that `issue1` is being modified by setting a body. `TestIssue::setBody` just calls through to the `TestIssueStore` object. The snapshot in `issue1` doesn't change. When the link is created it points to this snapshot. When `issue2.links()` is called, the new logic will return a new `Link` object pointing to a fresh snapshot of `issue1`. If we just ran `equals` to compare `link` with `links.get(0)`, then it would return false, because `link` points to `issue1` without a body, while `links.get(0)` points to a fresh snapshot with a body.

Checking that `Issue::id` is equal is the correct thing to do here. Links point to an issue identified by an ID, not to a specific snapshot of the issue.

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

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