diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b37e02d6..1c66e27f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -137,3 +137,38 @@ You can run the `make spell` command for this. `make doublecheck` will run the code formatter, spell checker and unit tests. + +### Functional tests: moving away from brittle ID hooks + +Historically the functional test suite relied on stabber's id based helpers like `stbbr_for_id("prof_presence_1", ...)` to register canned responses that would be sent once Profanity emitted a stanza carrying that exact `id` attribute. This made the tests fragile: + +* Changes to stanza id generation (sequence, format) broke tests unexpectedly. +* Reordering internal requests produced hard-to-debug race conditions when an `id` no longer matched. +* Parallel additions of new features could shift which stanzas received a given id causing unrelated test failures. + +We have migrated to content based stubbing using direct sends (`stbbr_send`) and query hooks (`stbbr_for_query`). Instead of tying a response to a predicted id we now send the required server stanzas explicitly after initiating actions. Example (see `tests/functionaltests/proftest.c`): + +```c +// Old brittle approach +stbbr_for_id("prof_presence_1", ""); + +// New approach: after authentication, send presence directly +stbbr_send("...caps..."); +``` + +Benefits: + +* Eliminates dependency on internal id sequencing. +* Clearer intent inside test code ("send presence now" vs "register hook and hope client triggers it"). +* Simplifies adding new tests—no need to inspect logs for generated ids. + +Guidelines when writing new functional tests: + +1. Prefer `stbbr_for_query(namespace, xml)` for IQ roster or disco queries where the namespace is stable. +2. Use `stbbr_send(xml)` for presence, message, and other push style stanzas. +3. Avoid `stbbr_for_id` unless the protocol flow genuinely requires correlating a specific request/response pair not covered by a namespace query. +4. Keep assertions tolerant of ordering when possible; rely on `prof_output_regex()` matches rather than hard-coded positions. +5. If timing flakiness appears, temporarily raise `prof_timeout()` around the critical expectation and reset it immediately afterwards. + +The migration from `stbbr_for_id` is complete. All functional tests now use content-based stubbing. When adding new tests, follow the guidelines above. + diff --git a/ci-build.sh b/ci-build.sh index 82ff76ff..0a3defb8 100755 --- a/ci-build.sh +++ b/ci-build.sh @@ -44,49 +44,43 @@ ARCH="$(uname | tr '[:upper:]' '[:lower:]')" case "$ARCH" in linux*) + # Reduced set of configurations for faster CI tests=( + # 1. Full build (all features enabled) "--enable-notifications --enable-icons-and-clipboard --enable-otr --enable-pgp --enable-omemo --enable-plugins --enable-c-plugins --enable-python-plugins --with-xscreensaver --enable-omemo-qrcode --enable-gdk-pixbuf" + # 2. Minimal build (all optional features disabled) "--disable-notifications --disable-icons-and-clipboard --disable-otr --disable-pgp --disable-omemo --disable-plugins --disable-c-plugins - --disable-python-plugins --without-xscreensaver" - "--disable-notifications" - "--disable-icons-and-clipboard" - "--disable-otr" - "--disable-pgp" - "--disable-omemo --disable-omemo-qrcode" - "--disable-pgp --disable-otr" - "--disable-pgp --disable-otr --disable-omemo" - "--disable-plugins" - "--disable-python-plugins" - "--disable-c-plugins" - "--disable-c-plugins --disable-python-plugins" - "--without-xscreensaver" - "--disable-gdk-pixbuf" - "") + --disable-python-plugins --without-xscreensaver --disable-omemo-qrcode --disable-gdk-pixbuf" + # 3. No encryption (disable otr, pgp, omemo) + "--disable-pgp --disable-otr --disable-omemo --disable-omemo-qrcode" + # 4. No plugins + "--disable-plugins --disable-c-plugins --disable-python-plugins" + # 5. Default configuration + "" + ) source /etc/profile.d/debuginfod.sh 2>/dev/null || true ;; darwin*) + # Reduced set of configurations for faster CI tests=( + # 1. Full build (all features enabled) "--enable-notifications --enable-icons-and-clipboard --enable-otr --enable-pgp --enable-omemo --enable-plugins --enable-c-plugins --enable-python-plugins" + # 2. Minimal build (all optional features disabled) "--disable-notifications --disable-icons-and-clipboard --disable-otr --disable-pgp --disable-omemo --disable-plugins --disable-c-plugins --disable-python-plugins" - "--disable-notifications" - "--disable-icons-and-clipboard" - "--disable-otr" - "--disable-pgp" - "--disable-omemo" - "--disable-pgp --disable-otr" + # 3. No encryption (disable otr, pgp, omemo) "--disable-pgp --disable-otr --disable-omemo" - "--disable-plugins" - "--disable-python-plugins" - "--disable-c-plugins" - "--disable-c-plugins --disable-python-plugins" - "") + # 4. No plugins + "--disable-plugins --disable-c-plugins --disable-python-plugins" + # 5. Default configuration + "" + ) ;; openbsd*) MAKE="gmake" @@ -96,25 +90,23 @@ case "$ARCH" in # src/event/server_events.c:1454:19: error: universal character names are only valid in C++ and C99 CC="egcc -std=gnu99 -fexec-charset=UTF-8" + # Reduced set of configurations for faster CI tests=( + # 1. Full build (all features enabled) "--enable-notifications --enable-icons-and-clipboard --enable-otr --enable-pgp --enable-omemo --enable-plugins --enable-c-plugins --enable-python-plugins" + # 2. Minimal build (all optional features disabled) "--disable-notifications --disable-icons-and-clipboard --disable-otr --disable-pgp --disable-omemo --disable-plugins --disable-c-plugins --disable-python-plugins" - "--disable-notifications" - "--disable-icons-and-clipboard" - "--disable-otr" - "--disable-pgp" - "--disable-omemo" - "--disable-pgp --disable-otr" + # 3. No encryption (disable otr, pgp, omemo) "--disable-pgp --disable-otr --disable-omemo" - "--disable-plugins" - "--disable-python-plugins" - "--disable-c-plugins" - "--disable-c-plugins --disable-python-plugins" - "") + # 4. No plugins + "--disable-plugins --disable-c-plugins --disable-python-plugins" + # 5. Default configuration + "" + ) ;; esac diff --git a/prof.supp b/prof.supp index c2f7f59c..87aca009 100644 --- a/prof.supp +++ b/prof.supp @@ -8,6 +8,83 @@ # * python suppressions file from https://github.com/python/cpython/blob/main/Misc/valgrind-python.supp # +# ============================================ +# Functional tests suppressions (stabber/pthread) +# ============================================ + +{ + stabber_pthread_create + Memcheck:Leak + match-leak-kinds: possible + fun:calloc + ... + fun:allocate_dtv + fun:_dl_allocate_tls + ... + fun:pthread_create* + ... + fun:server_run + ... +} + +{ + stabber_server_run + Memcheck:Leak + match-leak-kinds: possible + fun:calloc + ... + fun:pthread_create* + ... + obj:*/libstabber* + ... +} + +{ + glib_time_zone_cache + Memcheck:Leak + match-leak-kinds: reachable + ... + fun:g_time_zone_new* + ... +} + +{ + glib_time_zone_local + Memcheck:Leak + match-leak-kinds: reachable + ... + fun:g_strdup + ... + fun:g_time_zone_new_identifier + fun:g_time_zone_new_local + ... +} + +{ + glib_date_time_format + Memcheck:Leak + match-leak-kinds: reachable + ... + fun:g_date_time_format + ... +} + +{ + glib_date_time_format_locale + Memcheck:Leak + match-leak-kinds: reachable + fun:*alloc + ... + obj:*/libglib* + ... + fun:g_date_time_format + ... +} + +# ============================================ +# Original suppressions +# ============================================ + { _dl_init Memcheck:Leak @@ -2703,3 +2780,284 @@ fun:calloc fun:_dl_allocate_tls } + +# pthread TLS allocation in stabber server threads +{ + pthread_create_tls_stabber + Memcheck:Leak + match-leak-kinds: possible + fun:calloc + ... + fun:allocate_dtv + fun:_dl_allocate_tls + fun:allocate_stack + fun:pthread_create* + fun:server_run +} + +# expect/tcl library allocations +{ + tcl_alloc_expect + Memcheck:Leak + match-leak-kinds: possible + fun:malloc + ... + fun:TclpAlloc + fun:Tcl_Alloc + ... + fun:exp_expectl +} + +{ + exp_printify + Memcheck:Leak + match-leak-kinds: possible + fun:malloc + ... + fun:exp_printify + ... + fun:exp_expectl +} + + +# Additional suppressions for functional tests + +# libexpect still reachable allocations +{ + exp_spawnv_malloc + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + ... + fun:exp_spawnv + fun:exp_spawnl +} + +{ + exp_spawnl_realloc + Memcheck:Leak + match-leak-kinds: reachable + fun:realloc + ... + fun:exp_spawnv + fun:exp_spawnl +} + +# libtcl memory pool allocations (expected) +{ + tcl_alloc_pool + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + ... + fun:TclpAlloc + fun:Tcl_Alloc +} + +{ + tcl_alloc_pool_calloc + Memcheck:Leak + match-leak-kinds: reachable + fun:calloc + ... + fun:TclpAlloc + fun:Tcl_Alloc +} + +# pthread thread-local storage (normal for multi-threaded programs) +{ + pthread_tls_allocate_dtv + Memcheck:Leak + match-leak-kinds: possible + fun:calloc + fun:calloc + fun:allocate_dtv + fun:_dl_allocate_tls + fun:allocate_stack + fun:pthread_create* +} + +# glib static initializations +{ + glib_hash_table_init + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + fun:g_malloc + fun:g_hash_table_new_full + ... + fun:call_init + fun:_dl_init +} + +{ + glib_array_init + Memcheck:Leak + match-leak-kinds: reachable + fun:realloc + fun:g_realloc + ... + fun:call_init + fun:_dl_init +} + +# fdopen/fopen allocations (FILE* buffers - normal) +{ + fdopen_file_buffer + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + ... + fun:fdopen* +} + +{ + fopen_file_buffer + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + ... + fun:fopen* +} + +# stabber log_init (server log file) +{ + stabber_log_init + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + ... + fun:log_init + fun:server_run +} + +# glib time zone (static allocation) +{ + g_time_zone_new_local + Memcheck:Leak + match-leak-kinds: reachable + ... + fun:g_time_zone_new_local +} + +{ + g_time_zone_array + Memcheck:Leak + match-leak-kinds: reachable + fun:realloc + fun:g_realloc + ... + fun:g_array_sized_new + fun:g_time_zone_new_identifier +} + +# stabber log g_date_time_format invalid read - benign race in logging +{ + stabber_g_date_time_format_invalid_read + Memcheck:Addr1 + ... + fun:g_date_time_format + fun:log_println + ... +} + +{ + stabber_g_date_time_format_invalid_read2 + Memcheck:Addr2 + ... + fun:g_date_time_format + fun:log_println + ... +} + +{ + stabber_g_date_time_format_invalid_read4 + Memcheck:Addr4 + ... + fun:g_date_time_format + fun:log_println + ... +} + +{ + stabber_g_date_time_format_invalid_read8 + Memcheck:Addr8 + ... + fun:g_date_time_format + fun:log_println + ... +} + +# More generic suppression for g_date_time_format race condition +{ + g_date_time_format_cond + Memcheck:Cond + ... + fun:g_date_time_format + ... +} + +{ + g_date_time_format_value1 + Memcheck:Value1 + ... + fun:g_date_time_format + ... +} + +{ + g_date_time_format_value2 + Memcheck:Value2 + ... + fun:g_date_time_format + ... +} + +{ + g_date_time_format_value4 + Memcheck:Value4 + ... + fun:g_date_time_format + ... +} + +{ + g_date_time_format_value8 + Memcheck:Value8 + ... + fun:g_date_time_format + ... +} + +# Suppress all Addr errors in log_println +{ + log_println_addr1 + Memcheck:Addr1 + ... + fun:log_println + ... +} + +{ + log_println_addr2 + Memcheck:Addr2 + ... + fun:log_println + ... +} + +{ + log_println_addr4 + Memcheck:Addr4 + ... + fun:log_println + ... +} + +{ + log_println_addr8 + Memcheck:Addr8 + ... + fun:log_println + ... +}