Refactor utests: replace internal functions with API and clean up by HanzlikPetr · Pull Request #2529 · CESNET/libyang

HanzlikPetr · 2026-06-10T11:21:24Z

No description provided.

Roytak

Noticed a couple of possible places for improvement, but very good overall.

Roytak · 2026-06-17T09:01:59Z

+    // array coresponds to the following YANG module:
+    // module test { namespace "urn:test"; prefix t; description "XXX"; }


Use block comment.

Roytak · 2026-06-17T09:31:05Z

+
+enum ly_path_pred_type {
+    LY_PATH_PREDTYPE_POSITION,  /**< position predicate - [2] */
+    LY_PATH_PREDTYPE_LIST,      /**< keys predicate - [key1='val1'][key2='val2']... */
+    LY_PATH_PREDTYPE_LEAFLIST,  /**< leaflist value predicate - [.='value'] */
+    LY_PATH_PREDTYPE_LIST_VAR   /**< keys predicate with variable instead of value - [key1=$USER]... */
+};



Just copying the enum from path.h verbatim feels very fragile, modifying this enum in path.h would break this test. Also the actual value comparison was removed from CHECK_LYD_VALUE_INST and only the array sizes are used - these values are never used. A plain int array would serve the same purpose. Also this same enum was also copied to union.c. My suggestion is to either:

add the value validation back to CHECK_LYD_VALUE_INST and #define these values in utests.h (based on a comment in path.h: "/**< Predicate type (see YANG ABNF) */", it would seem that these values are standardized, but I am not sure about that, you may try to find out more about it)

remove the enums from the tests completely, since the values are not checked anyways, and just pass integer arrays to CHECK_LYD_VALUE_INST

Roytak · 2026-06-17T09:31:38Z

+
+enum ly_path_pred_type {
+    LY_PATH_PREDTYPE_POSITION,  /**< position predicate - [2] */
+    LY_PATH_PREDTYPE_LIST,      /**< keys predicate - [key1='val1'][key2='val2']... */
+    LY_PATH_PREDTYPE_LEAFLIST,  /**< leaflist value predicate - [.='value'] */
+    LY_PATH_PREDTYPE_LIST_VAR   /**< keys predicate with variable instead of value - [key1=$USER]... */
+};


Related to the comment in instanceid.c.

Roytak · 2026-06-17T09:39:49Z

 #define CHECK_LYSC_TYPE(NODE, TYPE, EXTS) \
    assert_non_null(NODE); \
    assert_int_equal((NODE)->basetype, TYPE); \
    CHECK_ARRAY((NODE)->exts, EXTS); \
-    assert_ptr_equal((NODE)->plugin_ref, lyplg_type_plugin_find(NULL, "", NULL, ly_data_type2str[TYPE]))
+    assert_non_null((NODE)->plugin_ref)


This checks seems much weaker, would lysc_get_type_plugin work here possibly?

Roytak · 2026-06-17T09:46:13Z

 */

+#define UNUSED(x) UNUSED_ ## x __attribute__((__unused__))
+


I see that the macro UNUSED is defined in compat.h, consider including it instead.

Roytak · 2026-06-17T09:52:15Z

-static void
-test_extension_compile(void **state)
-{
-    struct lys_module *mod;


Can this not be rewritten via public API? For example parsing a module with md:annotation containing substatements (or other ext like structure), then inspecting the compiled lysc_ext_instance and verifying the compiled substatement could possibly work.

Roytak · 2026-06-17T09:56:01Z

+    const char *invalid[] = {
+        "invalid_path",
+        "..[",
+        "../",
+        "/",
+        "../../pref:id/xxx[predicate]/invalid!!!"
+    };


Is there a reason why ".." was excluded? It was tested previously.

Roytak · 2026-06-17T09:58:06Z

-    v1 = "::C:D:E:f:a/96";
+    type = lysc_get_type_plugin(lysc_type->plugin_ref);
+    v1 = "::C:D:E:f:a/112";
    assert_int_equal(LY_SUCCESS, type->store(UTEST_LYCTX, lysc_type, v1, strlen(v1) * 8,


Why were the values swapped here?

Roytak · 2026-06-17T10:01:11Z

 static void
 test_lyds_free_metadata(void **state)
 {
    const char *schema;
    struct lys_module *mod;
    struct lyd_node *first, *node;

    schema = "module a {namespace urn:tests:a;prefix a;yang-version 1.1;revision 2014-05-08;"
            "leaf-list ll {type uint32;}}";
    UTEST_ADD_MODULE(schema, LYS_IN_YANG, NULL, &mod);

    assert_int_equal(lyd_new_term(NULL, mod, "ll", "1", 0, &first), LY_SUCCESS);
    assert_int_equal(lyd_new_term(NULL, mod, "ll", "2", 0, &node), LY_SUCCESS);
    assert_int_equal(lyd_insert_sibling(first, node, NULL), LY_SUCCESS);
-    lyds_free_metadata(first);
-    assert_null(first->meta);
-    assert_null(node->meta);

    lyd_free_all(first);
 }


It seems this test doesn't really test anything now, consider removing/adjusting it.

Roytak · 2026-06-17T10:09:29Z

 #define CHECK_LYD_VALUE_INST(NODE, CANNONICAL_VAL, VALUE) \
    assert_non_null(lyd_value_get_canonical(UTEST_LYCTX, &(NODE))); \
    assert_string_equal((NODE)._canonical, CANNONICAL_VAL); \
    assert_non_null((NODE).realtype); \
    assert_int_equal(LY_TYPE_INST, (NODE).realtype->basetype); \
    { \
        LY_ARRAY_COUNT_TYPE arr_size = sizeof(VALUE) / sizeof(VALUE[0]); \
        assert_int_equal(arr_size, LY_ARRAY_COUNT((NODE).target)); \
-        for (LY_ARRAY_COUNT_TYPE it = 0; it < arr_size; it++) { \
-            if ((NODE).target[it].predicates) { \
-                assert_int_equal(VALUE[it], (NODE).target[it].predicates[0].type); \
-            } \
-        } \
    }


See comment in instanceid.c.

Petr Hanzlik added 2 commits June 10, 2026 12:43

utests basic UPDATE replace internal functions with API

496b773

utests data UPDATE replace internal functions with API

ec9fa30

HanzlikPetr force-pushed the unit_test_change branch from f01e360 to 953755a Compare June 10, 2026 11:44

HanzlikPetr changed the title ~~Refactor utests to replace internal functions with API and clean up~~ Refactor utests: replace internal functions with API and clean up Jun 10, 2026

Petr Hanzlik added 3 commits June 11, 2026 10:04

utests schema UPDATE replace internal funstions with API

4573c39

utests type UPDATE replace internal funstions with API

862c87a

utests UPDATE remove unused headers

b6a7042

HanzlikPetr force-pushed the unit_test_change branch from 953755a to b6a7042 Compare June 11, 2026 08:07

Roytak reviewed Jun 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor utests: replace internal functions with API and clean up#2529

Refactor utests: replace internal functions with API and clean up#2529
HanzlikPetr wants to merge 5 commits into
CESNET:develfrom
HanzlikPetr:unit_test_change

HanzlikPetr commented Jun 10, 2026

Uh oh!

Roytak left a comment

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Roytak Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// array coresponds to the following YANG module:
		// module test { namespace "urn:test"; prefix t; description "XXX"; }

		*/

		#define UNUSED(x) UNUSED_ ## x __attribute__((__unused__))

Conversation

HanzlikPetr commented Jun 10, 2026

Uh oh!

Roytak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants