add support for list network area and test (#381) #387

larrytamnjong · 2024-10-28T08:57:20Z

No description provided.

larrytamnjong · 2024-10-28T09:01:41Z

marcin-krystianc · 2024-10-29T09:56:02Z

Consul.Test/OperatorTest.cs

+        public async Task Operator_AreaList()
+        {
+            var req = await _client.Operator.AreaList();
+            Assert.NotNull(req.Response);


To make this test more exhaustive, it would be nice to create an area first and then assert that the query result is non-empty (instead of not null).

Thanks, done ✅

Pleas also updated the test condition, from not null to non-empty. Please inspect also a result fields if they are filled in.

marcin-krystianc

The test are failing now with:

Consul.Test.OperatorTest.Operator_CreateArea: Consul.ConsulRequestException : Unexpected response, status code InternalServerError: area "d0c9af6b-363b-69b3-a1a9-50089871df13" with peer datacenter "dc2" already exists
            Assert.NotNull(response.Response);	            Assert.NotNull(response.Response);
        }	        }
        [EnterpriseOnlyFact]
        public async Task Operator_AreaList()
        {
            await _client.Operator.CreateArea(new AreaRequest { PeerDatacenter = "dc2", UseTLS = false, RetryJoin = null });

            var req = await _client.Operator.AreaList();
            Assert.NotNull(req.Response);
Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc 3 hours ago
To make this test more exhaustive, it would be nice to create an area first and then assert that the query result is non-empty (instead of not null).

Contributor
Author
@[larrytamnjong](https://github.com/larrytamnjong) larrytamnjong 29 minutes ago
Thanks, done ✅

Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc now
Pleas also updated the test condition, from not null to non-empty. Please inspect also a result fields if they are filled in.

@marcin-krystianc	Reply...
        }
    }	    }
}	}

Maybe try using a random name every time you create an area.

larrytamnjong · 2024-10-29T14:13:43Z

The test are failing now with:

Consul.Test.OperatorTest.Operator_CreateArea: Consul.ConsulRequestException : Unexpected response, status code InternalServerError: area "d0c9af6b-363b-69b3-a1a9-50089871df13" with peer datacenter "dc2" already exists
            Assert.NotNull(response.Response);	            Assert.NotNull(response.Response);
        }	        }
        [EnterpriseOnlyFact]
        public async Task Operator_AreaList()
        {
            await _client.Operator.CreateArea(new AreaRequest { PeerDatacenter = "dc2", UseTLS = false, RetryJoin = null });

            var req = await _client.Operator.AreaList();
            Assert.NotNull(req.Response);
Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc 3 hours ago
To make this test more exhaustive, it would be nice to create an area first and then assert that the query result is non-empty (instead of not null).

Contributor
Author
@[larrytamnjong](https://github.com/larrytamnjong) larrytamnjong 29 minutes ago
Thanks, done ✅

Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc now
Pleas also updated the test condition, from not null to non-empty. Please inspect also a result fields if they are filled in.

@marcin-krystianc	Reply...
        }
    }	    }
}	}

Maybe try using a random name every time you create an area.

Thanks, @marcin-krystianc your right the error occurred because I tried adding an operator area with the name "dc2," which was already created during the "create operator area" test. I’ve fixed that issue now. Hopefully, it's okay now.

marcin-krystianc · 2024-10-29T14:24:28Z

The test are failing now with:

Consul.Test.OperatorTest.Operator_CreateArea: Consul.ConsulRequestException : Unexpected response, status code InternalServerError: area "d0c9af6b-363b-69b3-a1a9-50089871df13" with peer datacenter "dc2" already exists
            Assert.NotNull(response.Response);	            Assert.NotNull(response.Response);
        }	        }
        [EnterpriseOnlyFact]
        public async Task Operator_AreaList()
        {
            await _client.Operator.CreateArea(new AreaRequest { PeerDatacenter = "dc2", UseTLS = false, RetryJoin = null });

            var req = await _client.Operator.AreaList();
            Assert.NotNull(req.Response);
Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc 3 hours ago
To make this test more exhaustive, it would be nice to create an area first and then assert that the query result is non-empty (instead of not null).

Contributor
Author
@[larrytamnjong](https://github.com/larrytamnjong) larrytamnjong 29 minutes ago
Thanks, done ✅

Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc now
Pleas also updated the test condition, from not null to non-empty. Please inspect also a result fields if they are filled in.

@marcin-krystianc	Reply...
        }
    }	    }
}	}

Maybe try using a random name every time you create an area.

Thanks, @marcin-krystianc your right the error occurred because I tried adding an operator area with the name "dc2," which was already created during the "create operator area" test. I’ve fixed that issue now. Hopefully, it's okay.

Using fixed name is not ideal, I guess it is impossible to run that test twice in a row? The test should be implemented in a way, that makes it possible to run it multiple times without any issue.

larrytamnjong · 2024-10-30T08:15:12Z

The test are failing now with:

Consul.Test.OperatorTest.Operator_CreateArea: Consul.ConsulRequestException : Unexpected response, status code InternalServerError: area "d0c9af6b-363b-69b3-a1a9-50089871df13" with peer datacenter "dc2" already exists
            Assert.NotNull(response.Response);	            Assert.NotNull(response.Response);
        }	        }
        [EnterpriseOnlyFact]
        public async Task Operator_AreaList()
        {
            await _client.Operator.CreateArea(new AreaRequest { PeerDatacenter = "dc2", UseTLS = false, RetryJoin = null });

            var req = await _client.Operator.AreaList();
            Assert.NotNull(req.Response);
Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc 3 hours ago
To make this test more exhaustive, it would be nice to create an area first and then assert that the query result is non-empty (instead of not null).

Contributor
Author
@[larrytamnjong](https://github.com/larrytamnjong) larrytamnjong 29 minutes ago
Thanks, done ✅

Member
@[marcin-krystianc](https://github.com/marcin-krystianc) marcin-krystianc now
Pleas also updated the test condition, from not null to non-empty. Please inspect also a result fields if they are filled in.

@marcin-krystianc	Reply...
        }
    }	    }
}	}

Maybe try using a random name every time you create an area.

Thanks, @marcin-krystianc your right the error occurred because I tried adding an operator area with the name "dc2," which was already created during the "create operator area" test. I’ve fixed that issue now. Hopefully, it's okay.

Using fixed name is not ideal, I guess it is impossible to run that test twice in a row? The test should be implemented in a way, that makes it possible to run it multiple times without any issue.

Thanks, @marcin-krystianc, and apologies for the back-and-forth. I’ve added a method to generate a random dc name for each test run. The tests are still in queue. Let me know if this approach works or if there's anything else you'd like me to adjust.

marcin-krystianc · 2024-10-30T11:20:05Z

Consul.Test/OperatorTest.cs

+        internal static string GeneratePeerDatacenterName()
+        {
+            int randomNumber = Random.Next(100, 1000);
+            return $"ran-dc-{randomNumber}";


Single random number might be not enough to prevent from conflicts/duplicates. We've already a method KVTest.GenerateTestKeyName() which is being used in many places in tests already. It generates 16 character long random string. Please use it instead.

@marcin-krystianc thanks, you can take another look now.

Consul.Test/OperatorTest.cs

marcin-krystianc · 2024-10-30T14:31:53Z

Consul.Test/OperatorTest.cs

+            await _client.Operator.CreateArea(new AreaRequest { PeerDatacenter = peerDataCenter, UseTLS = false, RetryJoin = null });
+
+            var req = await _client.Operator.AreaList();
+            Assert.NotEmpty(req.Response);


Just one more thing, can we test if the fields in the response are not null?
So far we know that we can create and list areas, but it would be great if we assert that the values returned by the list operation correspond to the values used in the create operation.

Can you also try filling the RetryJoin field and then validating the content of it when returned?

Consul/Interfaces/IOperatorEndpoint.cs

marcin-krystianc · 2024-10-31T08:06:39Z

Consul.Test/OperatorTest.cs

+
+            var req = await _client.Operator.AreaList();
+            Assert.NotEmpty(req.Response);
+            Assert.Equal(area.PeerDatacenter, req.Response.First().PeerDatacenter);


There can be multiple items in the result list, so the first one is not necessarily the one we are looking for. You need to look for the right one, e.g.:

var result = req.Response.Single(x => x.PeerDatacenter == area.peerDataCenter); Assert.Equal(area.UseTLS, result.UseTLS); Assert.Equal(area.RetryJoin, result.RetryJoin);

@marcin-krystianc I'm not sure if testing RetryJoin is sufficient with this approach Assert.Equal(area.RetryJoin, result.RetryJoin);
or if it would be better to test each element individually in the array, like this: for (int i = 0; i < retryJoinAddresses.Length; i++) { Assert.Equal(retryJoinAddresses[i], createdArea.RetryJoin[i]); }

@marcin-krystianc I'm not sure if testing RetryJoin is sufficient with this approach Assert.Equal(area.RetryJoin, result.RetryJoin); or if it would be better to test each element individually in the array, like this: for (int i = 0; i < retryJoinAddresses.Length; i++) { Assert.Equal(retryJoinAddresses[i], createdArea.RetryJoin[i]); }

There is an overload for IEnumerable:

public static void Equal<T>( IEnumerable<T> expected, IEnumerable<T> actual, IEqualityComparer<T> comparer)

so I guess it will do the right thing.
You can try to define manually two lists and see how it reacts to different item order in both lists.

Tests 1 and 3 are passing, but test 2 is failing so its ok. I have pushed the changes please let me know if there's anything else to change.

marcin-krystianc

It's perfect now 👍

larrytamnjong and others added 2 commits October 28, 2024 09:56

add support for list network area and test (G-Research#381)

d8f7051

style: fix whitespaces

20454fa

marcin-krystianc reviewed Oct 29, 2024

View reviewed changes

improve test result assertions by creating area beforehand

b2fe3b4

marcin-krystianc reviewed Oct 29, 2024

View reviewed changes

larrytamnjong added 2 commits October 29, 2024 14:24

resolve duplicate area creation in tests

3f20b91

update operator test

7d17b93

add function to generate random dc name in operator test

1fbda32

marcin-krystianc reviewed Oct 30, 2024

View reviewed changes

replace GeneratePeerDatacenterName() with KVTest.GenerateTestKeyName()

2b3e663

marcin-krystianc reviewed Oct 30, 2024

View reviewed changes

larrytamnjong added 4 commits October 30, 2024 18:45

rename CreateArea to AreaCreate

e3df4fc

add test to assert values returned by the list

a33a882

update test

88c1e3f

update test

be7b96b

marcin-krystianc reviewed Oct 31, 2024

View reviewed changes

update test

04a673a

marcin-krystianc approved these changes Oct 31, 2024

View reviewed changes

marcin-krystianc merged commit 18ba35f into G-Research:master Oct 31, 2024
243 checks passed

larrytamnjong deleted the list-network-areas branch October 31, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for list network area and test (#381) #387

add support for list network area and test (#381) #387

larrytamnjong commented Oct 28, 2024

larrytamnjong commented Oct 28, 2024

marcin-krystianc Oct 29, 2024

larrytamnjong Oct 29, 2024

marcin-krystianc Oct 29, 2024

marcin-krystianc left a comment

larrytamnjong commented Oct 29, 2024

marcin-krystianc commented Oct 29, 2024

larrytamnjong commented Oct 30, 2024

marcin-krystianc Oct 30, 2024

larrytamnjong Oct 30, 2024

marcin-krystianc Oct 30, 2024

larrytamnjong Oct 30, 2024

marcin-krystianc Oct 31, 2024

marcin-krystianc Oct 31, 2024

larrytamnjong Oct 31, 2024

marcin-krystianc Oct 31, 2024

larrytamnjong Oct 31, 2024

marcin-krystianc left a comment •

edited

Loading

add support for list network area and test (#381) #387

add support for list network area and test (#381) #387

Conversation

larrytamnjong commented Oct 28, 2024

larrytamnjong commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-krystianc left a comment

Choose a reason for hiding this comment

larrytamnjong commented Oct 29, 2024

marcin-krystianc commented Oct 29, 2024

larrytamnjong commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-krystianc left a comment • edited Loading

Choose a reason for hiding this comment

marcin-krystianc left a comment •

edited

Loading