[GitHub] [geode] Bill commented on a change in pull request #5370: GEODE-8298: Member version comparison sense inconsistent when deciding on multicast

[GitBox <gi...@apache.org>]

Bill commented on a change in pull request #5370:
URL: https://github.com/apache/geode/pull/5370#discussion_r455147203



##########
File path: geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
##########
@@ -328,4 +332,63 @@ public void noDispatchWhenSick() throws MemberShunnedException, MemberStartupExc
     assertThat(spy.getStartupEvents()).isEmpty();
   }
 
+  @Test
+  public void testIsMulticastAllowedWithOldVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(Version.CURRENT);
+    manager.addSurpriseMember(createSurpriseMember(OLDER_THAN_CURRENT_VERSION));
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isFalse();
+  }
+
+  @Test
+  public void testIsMulticastAllowedWithCurrentVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(Version.CURRENT);
+    manager.addSurpriseMember(createSurpriseMember(Version.CURRENT));
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+  }
+
+  @Test
+  public void testIsMulticastAllowedWithOldVersionViewMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(OLDER_THAN_CURRENT_VERSION);
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isFalse();
+  }
+
+  @Test
+  public void testMulticastAllowedWithCurrentVersionViewMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(Version.CURRENT);
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+  }
+
+  private MemberIdentifier createSurpriseMember(Version version) {
+    MemberIdentifier surpriseMember = createMemberID(DEFAULT_PORT + 5);
+    surpriseMember.setVmViewId(3);
+    surpriseMember.setVersionObjectForTest(version);
+    return surpriseMember;
+  }
+
+  private MembershipView<MemberIdentifier> createMembershipView(Version version) {
+    List<MemberIdentifier> viewMembers = createMemberIdentifiers(version);
+    return new MembershipView<>(myMemberId, 2, viewMembers);
+  }
+
+  private List<MemberIdentifier> createMemberIdentifiers(Version memberVersion) {
+    List<MemberIdentifier> viewMembers = new ArrayList<>();
+    for (int i = 0; i < 2; ++i) {
+      MemberIdentifier memberIdentifier = createMemberID(DEFAULT_PORT + 6 + i);
+      memberIdentifier.setVersionObjectForTest(memberVersion);

Review comment:
       A small point: this routine will set _every_ member's version to `memberVersion`. I think it would be a better test if only _one_ member's version was set to `memberVersion` and the rest were set to `Version.CURRENT`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org