On Thu, 2014-07-10 at 13:39 +0200, Pavel Březina wrote:
Hi,
thanks for the unit test.
Would you consider braking test into smaller part to make it possible to
run it over different sets of test data?
Please see comments inline.
Thanks,
Pavel R
+START_TEST(test_resolv_sort_srv_reply_zero_weight)
+{
+ int ret;
+ struct ares_srv_reply *replies = NULL;
+ struct ares_srv_reply *r, *prev = NULL;
+ struct resolv_test_ctx *test_ctx;
+ int num_replies = 6;
+ int i;
+
+ ret = setup_resolv_test(RESOLV_DEFAULT_TIMEOUT, &test_ctx);
+ if (ret != EOK) {
+ fail("Could not set up test");
+ return;
+ }
+
+ ck_leaks_push(test_ctx);
+
+ /* prepare linked list with reversed values */
^^^^^^^^
How do you mean reverse? Do you mean descending order? I think priorities are rather in
ascending order something like this:
(replies,0) -> (_,0) -> (_,10) -> (_,10) -> (_,20) -> (_,20)
If I am right could you amend the comment?
+ for (i = 0; i < num_replies; i++) {
+ r = talloc_zero(test_ctx, struct ares_srv_reply);
+ fail_if(r == NULL);
+
+ r->priority = 20;
+ r->priority = i <= 3 ? 10 : r->priority;
+ r->priority = i <= 1 ? 0 : r->priority;
+ r->weight = 0;
+
+ if (replies == NULL) {
+ replies = r;
+ prev = r;
+ } else {
+ prev->next = r;
+ prev = prev->next;
Would it make sense to use here macros from DLIST?
Ah...I see it's not used anywhere in this module anyway, so I guess it would be
inconsistent to start using it here.
+ }
+ }
+
+ /* do the sort */
+ ret = resolv_sort_srv_reply(&replies);
Are you sure about the input data? I think it's already sorted, is it on purpose?
I think it's perfectly OK to test it on sorted data, but I would also like to see it
run on mixed data, would it make sense to you?
+ fail_if(ret != EOK);
+
+ /* check if the list contains all values and is sorted */
+ for (i = 0, r = replies; r != NULL; r = r->next, i++) {
+ if (r->next != NULL) {
+ fail_unless(r->priority <= r->next->priority);
+ }
+ }
+ fail_unless(i == num_replies);
+
+ /* clean up */
+ prev = NULL;
+ for (i = 1, r = replies; r != NULL; r=r->next, i++) {
Do I miss something or is 'i' used in for-loop by accident?
+ talloc_zfree(prev);
+ prev = r;
+ }
+ talloc_zfree(prev);
+
+
+ /* check for leaks */
+ ck_leaks_pop(test_ctx);
+ talloc_zfree(test_ctx);
+}
+END_TEST
+
START_TEST(test_resolv_free_req)
{
int ret = EOK;
@@ -904,6 +967,7 @@ Suite *create_resolv_suite(void)
tcase_add_test(tc_resolv, test_address_to_string);
tcase_add_test(tc_resolv, test_resolv_ip_addr);
tcase_add_test(tc_resolv, test_resolv_sort_srv_reply);
+ tcase_add_test(tc_resolv, test_resolv_sort_srv_reply_zero_weight);
if (use_net_test) {
tcase_add_test(tc_resolv, test_resolv_internet);
tcase_add_test(tc_resolv, test_resolv_negative);