RFR: 1627: Add tests for bot factories [v2]

[Erik Joelsson]

On Wed, 12 Oct 2022 15:52:02 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> bots/csr/src/test/java/org/openjdk/skara/bots/csr/CSRBotFactoryTest.java line 178:
>> 
>>> 176:         }
>>> 177:     }
>>> 178: }
>> 
>> I tried to write the test with `TestBotFactory`, but I still couldn't get the advantage of using `TestBotFactory`.
>> 
>> **Using `TestBotRunnerConfiguration`:**
>> pros:
>> 1. We just override two parsers in `BotRunnerConfiguration`, so we could go through most of code in real `BotRunnerConfiguration` and `BotConfiguration`.
>> 2. We don't need to create RepositoryHost or IssueHost manually.
>> 
>> cons:
>> 1. we need the config of forges and issueTrackers
>> --------------------------------------------------------------------------------------------------------------
>> 
>> **Using `TestBotFactory`**:
>> pros:
>> 1. we don't need the config of forges and issueTrackers
>> 
>> cons:
>> 1. we need to create all HostedRepositories and issueProjects manually because the fields in TestBotFactory is `private final Map<String, HostedRepository> hostedRepositories;` & `private final Map<String, IssueProject> issueProjects;`, otherwise, the contents in the bots will be wrong.
>> 2.  `BotConfiguration` in `TestBotFactory` is not the real one we use.
>> --------------------------------------------------------------------------------------------------------------
>> 
>> All in all, I think use the `TestBotRunnerConfiguration` will be more realistic.
>
> Hi @erikj79 , if you have any time, please have a look at this comment.

I still think that using the already existing TestBotFactory is the way to go. Here are my reasons:

1. Adding another test implementation doing something very similar to what we already have is generally bad from a future maintenance perspective.
2. Having to open up the production class `BotRunnerConfiguration` for subclassing, just to be able to create a test class is sometimes a necessary pattern, but should generally be avoided if possible.
3. There is no need to create a `RepositoryHost` and `IssueHost` to instantiate `TestRepository` or `TestIssue`, you can just null those fields using the public constructor. We also don't need a local `Repository` instance (if we fix the constructor in `TestRepository` to stop calling `::url`).
4. I don't want these unit tests to need JSON definitions of forges and issue trackers. Especially if those need to fake actual github and jira configurations. That doesn't add any value to the tests, but makes these tests depend on how those entities are configured. The scope of the tests in question is to verify the behavior of the specific `*BotFactory` class, not parsing any other parts of a configuration file. 
5. Needing to create the `HostedRepository` and IssueProject` instances programmatically is actually a feature here. As an example, allows us to verify that the factory picks up the correct `RemoteRepository`, as the JSON does not allow us to define specific repositories, just a host.
6. Having a test implementation of `BotConfiguration` being used in these tests is a good thing. It makes it easier to customize specific behavior in a test to check corner cases. This is what unit tests are for. Granted, the current implementation is an inline class in TestBotFactory, but that can be refactored if needed. As I mentioned in an earlier comment, if we had access to a mock framework here, I would prefer mocking the `BotConfiguration` completely and just call the `*BotFactory::create` directly, but as we don't have such a dependency present in this project, using the existing test implementation classes is the next best thing.

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

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