Broker/Channel extension and tests - [merged] #131

Closed jdonkervliet closed 4 years ago

In GitLab by @swabbur on May 9, 2020, 15:46

Merges feature/pub-sub-tests -> development

Add isEmpty and isSubscribed methods to the Broker and Channel interfaces to enhance their usability. Also, add (unit) tests for both the new and existing functionality of these interfaces and their implementations.

In GitLab by @larsdetombe on May 9, 2020, 16:35

Commented on src/main/java/net/glowstone/messaging/Broker.java line 21

topicExists might be a better name?

In GitLab by @larsdetombe on May 9, 2020, 16:37

Commented on src/main/java/net/glowstone/messaging/concurrent/ConcurrentChannel.java line 33

I know you added this so testing might be easier, however shouldn't this method also be tested? Or do we think that it is not necessary?

In GitLab by @larsdetombe on May 9, 2020, 16:39

Commented on src/test/java/net/glowstone/messaging/BrokerTest.java line 19

This just checks whether a topic exists called 'Topic' right? not whether the broker does not contain any topics. Isn't that where the isEmpty() method could work better or did I misunderstand something?

In GitLab by @swabbur on May 9, 2020, 17:08

Commented on src/test/java/net/glowstone/messaging/BrokerTest.java line 19

Correct, that's why I was wondering whether an 'isEmpty()' like method should be added to the 'Broker' interface.

In GitLab by @swabbur on May 9, 2020, 17:10

Commented on src/main/java/net/glowstone/messaging/concurrent/ConcurrentChannel.java line 33

They weren't solely added to improve testing. It was also added to make debugging easier and to make the updateSubscriptions method in GlowWorld better readable and less erorr prone (I haven't made any changes there yet).

Also, it is "implicitely" tested in the subscribe and unsubscribe tests. I think that is sufficient.

In GitLab by @swabbur on May 9, 2020, 17:11

Commented on src/main/java/net/glowstone/messaging/Broker.java line 21

I'm mostly doubting about the use of exists, as a topic may still exist when there are no subscribers. Would inUse or isActive be better?

In GitLab by @larsdetombe on May 9, 2020, 17:18

Commented on src/main/java/net/glowstone/messaging/Broker.java line 21

If that is the case I would go for inUse

In GitLab by @larsdetombe on May 9, 2020, 17:20

Commented on src/test/java/net/glowstone/messaging/BrokerTest.java line 19

I think adding such a method would be a benefit, this test would also improve :)

In GitLab by @swabbur on May 9, 2020, 21:17

Commented on src/main/java/net/glowstone/messaging/Broker.java line 21

changed this line in version 2 of the diff

In GitLab by @swabbur on May 9, 2020, 21:17

Commented on src/test/java/net/glowstone/messaging/BrokerTest.java line 19

changed this line in version 2 of the diff

In GitLab by @swabbur on May 9, 2020, 21:17

added 4 commits

40ee9087 - Replace Broker's exists method with an isEmpty() method
8fa50e5a - Add return value to the (un)subscribe method and clean up JavaDoc
4092bf6d - Improve Channel tests
a9541865 - Clean up Broker tests

Compare with previous version

In GitLab by @swabbur on May 9, 2020, 21:20

Commented on src/test/java/net/glowstone/messaging/BrokerTest.java line 19

I've added the isEmpty() method and rewrote the test(s).

In GitLab by @swabbur on May 9, 2020, 21:21

Commented on src/main/java/net/glowstone/messaging/Broker.java line 21

I've completely removed the method as it implies the existence of some sub-grouping or channel, which isn't necessarily required in a Broker.

In GitLab by @swabbur on May 9, 2020, 21:21

resolved all threads

In GitLab by @JimVliet on May 10, 2020, 21:57

Commented on src/test/java/net/glowstone/messaging/BrokerTest.java line 21

I think a comment explaining the general test structure would be appropriate.

In GitLab by @JimVliet on May 10, 2020, 22:00

Commented on src/test/java/net/glowstone/messaging/ChannelTest.java line 19

Here a comment would be appropriate too.

In GitLab by @JimVliet on May 10, 2020, 22:04

Commented on src/main/java/net/glowstone/messaging/Broker.java line 20

Can you update the description of the MR slightly, to clarify that not only tests were added, but also that the Broker interface was modified.

In GitLab by @JimVliet on May 10, 2020, 22:05

Commented on src/main/java/net/glowstone/messaging/Broker.java line 20

Also mention the channel interface.

In GitLab by @swabbur on May 11, 2020, 11:16

added 1 commit

da2400e6 - Clarify test structure using JavaDoc

Compare with previous version

In GitLab by @swabbur on May 11, 2020, 11:19

changed title from {-Pub/Sub-} tests to {+Broker/Channel extension and+} tests

In GitLab by @swabbur on May 11, 2020, 11:19

changed the description

In GitLab by @swabbur on May 11, 2020, 11:19

Commented on src/test/java/net/glowstone/messaging/BrokerTest.java line 21

I've added JavaDoc to the class declaration and createBroker method to clarify their use and purpose.

In GitLab by @swabbur on May 11, 2020, 11:19

Commented on src/test/java/net/glowstone/messaging/ChannelTest.java line 19

Same here.

In GitLab by @swabbur on May 11, 2020, 11:20

Commented on src/main/java/net/glowstone/messaging/Broker.java line 20

I've specified the interfaces extended and tested in the MR description.

In GitLab by @JimVliet on May 11, 2020, 11:21

resolved all threads

In GitLab by @JimVliet on May 11, 2020, 11:22

enabled an automatic merge when the pipeline for da2400e6cbd48227a8cb34b180cc1587c97720ca succeeds

In GitLab by @JimVliet on May 11, 2020, 11:25

merged

In GitLab by @JimVliet on May 11, 2020, 11:25

mentioned in commit dd14677d80f0c914279b78f2cbaa3440b2e3ede2

atlarge-research / opencraft

Broker/Channel extension and tests - [merged] #131