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

[Erik Joelsson]

On Mon, 17 Oct 2022 17:05:08 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> Add some tests for BotFactories.
>
> Zhao Song 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 four additional commits since the last revision:
> 
>  - Adjusted format
>  - Added all botFactory tests
>  - Merge branch 'master' into SKARA-1627
>  - added some botFactory tests

This is starting to look pretty good. In several bots, there is more configuration data that needs verification, and to do that we need to add accessors to the bot class. I didn't comment individually on all those tests as it turned out to be quite a few. There are also some bots that aren't really used, so we shouldn't spend more time making the tests better for them, but these I did mark.

bot/src/main/java/org/openjdk/skara/bot/BotRunnerConfiguration.java line 46:

> 44: 
> 45: public class BotRunnerConfiguration {
> 46:     protected final Logger log;

This file needs to be restored.

bots/bridgekeeper/src/test/java/org/openjdk/skara/bots/bridgekeeper/BridgekeeperBotFactoryTest.java line 13:

> 11:     @Test
> 12:     public void testCreate() {
> 13:         var mirror1 = new TestHostedRepository(null, "mirror1");

I would suggest inlining the creation of all these repos (or any other support entity) unless you need to access them later in the test. If you need to create local variables for them, then I would like the `TestBotFactory` creation to be closer. I think having the test start with the JSON string makes for good readability.

bots/hgbridge/src/test/java/org/openjdk/skara/bots/hgbridge/JBridgeBotFactoryTest.java line 69:

> 67:             assertEquals("JBridgeBot at https://test.org/source1", jBridgeBot1.toString());
> 68:             JBridgeBot jBridgeBot2 = (JBridgeBot) bots.get(1);
> 69:             assertEquals("JBridgeBot at https://test.org/source2", jBridgeBot2.toString());

I think this bot is a good example of where we should probably add some accessors to be able to verify the configuration in more detail.

bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotFactoryTest.java line 152:

> 150:             assertEquals("MailingListBridgeBot at repo3", mailingListBridgeBots.get(0).toString());
> 151:             assertEquals("MailingListBridgeBot at repo4", mailingListBridgeBots.get(1).toString());
> 152:             assertEquals("MailingListBridgeBot at repo5", mailingListBridgeBots.get(2).toString());

It looks like there are more fields we would want to verify here too.

bots/submit/src/test/java/org/openjdk/skara/bots/submit/SubmitBotFactoryTest.java line 10:

> 8: import static org.junit.jupiter.api.Assertions.*;
> 9: 
> 10: class SubmitBotFactoryTest {

This bot isn't actually used, so no need to add more to this test.

bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestBotFactoryTest.java line 11:

> 9: import static org.junit.jupiter.api.Assertions.*;
> 10: 
> 11: class TestBotFactoryTest {

This bot isn't actually used, so no need to add more to this test.

test/src/main/java/org/openjdk/skara/test/TestBotFactory.java line 160:

> 158:     }
> 159: 
> 160:     public List<Bot> createBots(String name, JSONObject configuration) {

This is a copy of the method above. They should share implementation. You can make `create` call `createBots` and keep lines 151 to 154 to convert the list.

test/src/main/java/org/openjdk/skara/test/TestHostedRepository.java line 58:

> 56:     }
> 57: 
> 58:     public TestHostedRepository(TestHost host, String projectName) {

Could you add a comment explaining the limitations of creating an instance with this constructor? Something like:
"Creates an instance without a backing local repository that will not support any actual repository interaction."

Also since we are always calling this with host=null, we might as well remove that parameter too.

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

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