Buildbot embedded unittests #2755

rbouqueau · 2024-02-12T12:46:14Z

No description provided.

+ add unittest clean artifacts target to Makefile

+ null ptr, empty string, verylong string, non-ascii buffer + modify makefile to compile uts against local libgpac not the one installed + add code to gf_sys_word_match() to handle null arguments

touatily · 2024-02-14T09:01:18Z

include/tests.h

+        ((expr)                                                      \
+         ? gf_fatal_assert(0)                                        \
+         : printf("Assertion failed: %s\nMessage: %s\n\tFile: %s\nLine: %d\nFunction: %s\n", #expr, msg, __FILE__, __LINE__, __ASSERT_FUNCTION)); \


In both scenarios, an assertion failure occurs. Within the if statement, checks_passed is incremented, but it results in a call to gf_fatal_assert.

I propose to remove all the elements in that section that are unused. @soheibthriber Let me know if you disagree.

touatily · 2024-02-14T09:02:55Z

include/tests.h

+        ((expr)                                                      \
+         ? gf_fatal_assert(0)                                        \
+         : (action, gf_fatal_assert(0)));                            \


Same here. in both cases, gf_fatal_assert is called.

touatily · 2024-02-14T09:08:57Z

include/tests.h

+            checks_passed++;                                         \
+        } else {                                                     \
+            checks_failed++;                                         \
+            gf_assert(0);                                            \


Is there any reason for alternating between gf_assert and gf_fatal_assert at times, given their similarity?

No but using similar names is confusing, for sure :/

touatily · 2024-02-14T09:17:48Z

include/tests.h

+#define assert_equal_str(str1, str2)                                 \
+    do {                                                             \
+        if (!strcmp(str1, str2)) {                                   \
+            if (verbose_ut) printf("Assertion passed: Value: (%s, %s), File: %s, Line: %d, Function: %s\n", #str1, #str2, __FILE__, __LINE__, __ASSERT_FUNCTION); \
+            checks_passed++;                                         \
+        } else {                                                     \
+            checks_failed++;                                         \
+            gf_assert(0);                                            \
+        }                                                            \
+    } while (0)


Could this be rewritten as follows?

Suggested change

#define assert_equal_str(str1, str2) \

do { \

if (!strcmp(str1, str2)) { \

if (verbose_ut) printf("Assertion passed: Value: (%s, %s), File: %s, Line: %d, Function: %s\n", #str1, #str2, __FILE__, __LINE__, __ASSERT_FUNCTION); \

checks_passed++; \

} else { \

checks_failed++; \

gf_assert(0); \

} \

} while (0)

#define assert_equal_str(str1, str2) assert_false(strcmp(str1, str2))

touatily · 2024-02-14T09:37:31Z

include/tests.h

+    } while (0)
+
+#define assert_false(expr) assert_true(!(expr))
+


Other macros could be added. Here are some suggestions:

Suggested change

#define assert_not_equal_str(str1, str2) assert_true(strcmp((str1), (str2)))

#define assert_equal(a, b) assert_true((a) == (b))

#define assert_greater(a, b) assert_true((a) > (b))

#define assert_greater_equal(a, b) assert_true((a) >= (b))

#define assert_less(a, b) assert_true((a) < (b))

#define assert_less_equal(a, b) assert_true((a) <= (b))

#define assert_not_null(ptr) assert_true((ptr) != NULL)

I like them.

Maybe I would put them in MAJ to make it clear this belongs to the UT framework (and not GPAC in general).

Also we could limit to ASSERT() and ASSERT_STR_EQ() I guess. Opinion?

touatily · 2024-02-14T11:46:04Z

unittests/README

@@ -0,0 +1,30 @@
+# Unit tests in GPAC


Please consider adding the .md extension to the README file to ensure it is recognized as a markdown file. This will improve readability and enable markdown rendering on platforms that support it.

Will do, thanks!

aureliendavid · 2024-03-12T09:49:18Z

include/tests.h

+            checks_passed++;                                         \
+        } else {                                                     \
+            checks_failed++;                                         \
+            gf_assert(0);                                            \


currently if a test fails the whole testing is aborted because of the assert:

unittests: ut_os_config_init.c:36: test_gf_sys_word_match: Assertion `0' failed. Aborted (core dumped)

and you don't get the summary with the number of tests passed/failed

That's right, it is a design choice. Usually unit tests are executed at every build. So this is kind of some incremental approach. However we could add the option for failure to be non-fatal. Adding this in a7d736d.

aureliendavid · 2024-03-12T09:49:50Z

include/tests.h

+            checks_failed++;                                         \
+        }                                                            \
+        ((expr)                                                      \
+         ? gf_fatal_assert(0)                                        \


why do we assert(0) is the expr is true here?

outdated comment

aureliendavid · 2024-03-12T09:51:31Z

include/tests.h

+extern int checks_passed;
+extern int checks_failed;
+
+static Bool verbose_ut = GF_FALSE;


is there a way to set this to true via compile flags / configure?

Will do. What option do you envision? At the moment we just have --unittests.

aureliendavid · 2024-03-12T11:40:26Z

unittests/build.sh

+int main(int argc, char *argv[])
+{'
+
+calls=$(find "$scriptDir/.." -path "*/unittests/*.c" | grep -v bin | xargs grep unittest | cut -d ":" -f 2)


since scriptDir contains "unittests" and -path checks the whole path, this will match every .c file (also maybe add a -type f to exclude directories)

maybe grep -v bin may exclude a bit too much stuff (maybe grep -v '/bin/' or use exclude features from find?)

It seems like readlink and realpath are non portable. I just put a 'cd' before 'find' in a8e7ddb.

aureliendavid · 2024-03-12T12:01:58Z

configure

@@ -2167,6 +2171,8 @@ if test $cpu = "mips"; then
 fi
 echo ""
 echo "** GPAC $version rev$revision Core Configuration **"
+echo "Unit Tests: $unit_tests ('make unit_tests')"
+echo "#define GF_STATIC static" >> $TMPH


I think this should also be defined in configuration.h (or setup.h?) for platforms that don't use configure/config.h ?

Added in 672f666. Should we also add a target in the msvc Solution ? xcode ? What's the usual way to add this?

…ile platforms)

…aureliendavid

…aureliendavid

rbouqueau · 2024-03-25T15:00:43Z

Note from today's call:

This is ok as a first UT runtime. Existing unittests can run on any runtime anyway.
@aureliendavid to have a look at how to get rid of the double build.
Remarks that UTs should not leverage the configure script but a compile-time define (with -D) instead or with var env (as asan does).
These defines could also allow to get rid of the static variables.
Failing with if (fatal_ut) gf_assert(0); doesn't display the report when failing.

@aureliendavid

…ort - CC @aureliendavid

rbouqueau · 2024-04-04T15:27:58Z

Possible additional discussed tests:

gpac -h dash doesn't return dasher.
48e6376

rbouqueau · 2024-04-30T15:12:47Z

@aureliendavid Are we ok now? https://buildbot.gpac.io/#/grid?branch=buildbot-embedded-unittests

aureliendavid · 2024-04-30T15:39:15Z

it seems ok on the https://github.com/gpac/gpac/tree/buildbot-embedded-unittests branch

since it's a different branch than the one in this PR, maybe create a new PR for the new branch with a reference to this PR so that we don't lose the discussion? (or you can just merge the main repo branch directly if you prefer)

rbouqueau · 2024-04-30T21:15:47Z

Both branches are now aligned. @jeanlf Please merge if ok for you!

rbouqueau requested review from aureliendavid, jeanlf and touatily February 12, 2024 12:46

rbouqueau and others added 11 commits March 1, 2024 08:55

add minimal unit tests framework

0e8a49a

add some flavoured assert:

82b2f01

+ add unittest clean artifacts target to Makefile

wip: add cmocka wrappers

09c81e1

test limit conditions of gf_sys_word_match():

a783c17

+ null ptr, empty string, verylong string, non-ascii buffer + modify makefile to compile uts against local libgpac not the one installed + add code to gf_sys_word_match() to handle null arguments

cleanup

b88f4dc

UTs: introduce crashing test due to wrong API

67dbc41

UT: add test on static fonction

7decbad

UTs: improve result readability

5549b0c

UTs: automatically detect failing tests

a6d9f72

UTs: operate on a separate build

eb30bf3

UTs: fix out-of-tree build

977cce3

rbouqueau force-pushed the buildbot-embedded-unittests branch from 66dc9ca to 977cce3 Compare March 1, 2024 13:13

rbouqueau marked this pull request as ready for review March 1, 2024 13:16

touatily reviewed Mar 18, 2024

View reviewed changes

rbouqueau added 4 commits March 18, 2024 16:58

UTs: rename README -> README.md

f447cdf

UTs: simplify floavoured asserts - thanks @touatily

6fe74ce

cleanup

027deb3

UTs: more cross-platform

9b7368e

aureliendavid reviewed Mar 19, 2024

View reviewed changes

rbouqueau added 3 commits March 19, 2024 13:44

UTs: fatal by default made explicit (and desactivable)

a7d736d

UTs: add GF_STATIC in configuration.h (for Win32 only - excluding mob…

672f666

…ile platforms)

UTs: 'cd' before 'find' otherwise it finds all the c files! - thanks @…

a8e7ddb

…aureliendavid

rbouqueau added 2 commits April 2, 2024 12:22

UTs: fix fct signature

b792bf2

UTs: fatal error should not call assert to be able to display the rep…

ddc0882

…ort - CC @aureliendavid

fix build when relying on static configurations

0e41abc

jeanlf merged commit b7c020d into gpac:master May 6, 2024
16 checks passed

rbouqueau mentioned this pull request May 6, 2024

Buildbot scte35 id3 markers #2748

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buildbot embedded unittests #2755

Buildbot embedded unittests #2755

rbouqueau commented Feb 12, 2024

touatily Feb 14, 2024

rbouqueau Mar 18, 2024

touatily Feb 14, 2024

touatily Feb 14, 2024

rbouqueau Mar 18, 2024

touatily Feb 14, 2024

touatily Feb 14, 2024

rbouqueau Mar 18, 2024

touatily Feb 14, 2024

rbouqueau Mar 18, 2024

aureliendavid Mar 12, 2024

rbouqueau Mar 19, 2024 •

edited

aureliendavid Mar 12, 2024

rbouqueau Mar 19, 2024

aureliendavid Mar 12, 2024

rbouqueau Mar 19, 2024 •

edited

aureliendavid Mar 12, 2024

rbouqueau Mar 19, 2024

aureliendavid Mar 12, 2024

rbouqueau Mar 19, 2024

rbouqueau commented Mar 25, 2024 •

edited

rbouqueau commented Apr 4, 2024

rbouqueau commented Apr 30, 2024

aureliendavid commented Apr 30, 2024

rbouqueau commented Apr 30, 2024

+#define assert_not_equal_str(str1, str2)        assert_true(strcmp((str1), (str2)))
+#define assert_equal(a, b)                      assert_true((a) == (b))
+#define assert_greater(a, b)                    assert_true((a) > (b))
+#define assert_greater_equal(a, b)              assert_true((a) >= (b))
+#define assert_less(a, b)                       assert_true((a) < (b))
+#define assert_less_equal(a, b)                 assert_true((a) <= (b))
+#define assert_not_null(ptr)                    assert_true((ptr) != NULL)

Buildbot embedded unittests #2755

Buildbot embedded unittests #2755

Conversation

rbouqueau commented Feb 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbouqueau Mar 19, 2024 • edited

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

rbouqueau Mar 19, 2024 • edited

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

rbouqueau commented Mar 25, 2024 • edited

rbouqueau commented Apr 4, 2024

rbouqueau commented Apr 30, 2024

aureliendavid commented Apr 30, 2024

rbouqueau commented Apr 30, 2024

rbouqueau Mar 19, 2024 •

edited

rbouqueau Mar 19, 2024 •

edited

rbouqueau commented Mar 25, 2024 •

edited