From 56ede1b45fdeed3ff614f43d889a8501b99f2ece Mon Sep 17 00:00:00 2001 From: "jabber.developer2" Date: Wed, 21 Jan 2026 14:56:28 +0300 Subject: [PATCH] refactor: address code review feedback Makefile.am: - Replace 4 hardcoded parallel commands with loop over FUNC_TEST_GROUPS - Easy to extend by adding group numbers to the variable functionaltests.c: - Remove test counts from comments (prone to becoming outdated) - Restore previous comment style with === separators for readability - Replace switch with groups[] array structure for extensibility proftest.h: - Fix comment to describe current state, not the change made CONTRIBUTING.md: - Document test groups and parallel execution commands - Add instructions for adding new test groups --- CONTRIBUTING.md | 22 ++++++- Makefile.am | 24 ++++--- tests/functionaltests/functionaltests.c | 86 +++++++++++++------------ tests/functionaltests/proftest.h | 6 +- 4 files changed, 84 insertions(+), 54 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 228cfb4d..a0ad4248 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -147,9 +147,28 @@ The functional test suite uses [stabber](https://git.jabber.space/devs/stabber) Functional tests require stabber to be installed. Once installed, tests run as part of `make check`: ```bash -make check +make check # Run all tests (unit + functional) +make check-functional-parallel # Run functional tests in parallel (~3x faster) +./tests/functionaltests/functionaltests # Run all functional tests sequentially +./tests/functionaltests/functionaltests 1 # Run specific group (1-4) ``` +#### Test groups + +Tests are organized into 4 groups for parallel execution: + +| Group | Description | +|-------|-------------| +| 1 | Connect, Ping, Rooms, Software | +| 2 | Message, Receipts, Roster, Chat Session | +| 3 | Presence, Disconnect | +| 4 | MUC, Carbons | + +To add a new group: +1. Define the test array in `functionaltests.c` +2. Add entry to `groups[]` array +3. Update `FUNC_TEST_GROUPS` in `Makefile.am` + #### Writing functional tests Use content-based stubbing with stabber: @@ -168,4 +187,5 @@ Guidelines: 2. Use `stbbr_send(xml)` for presence, message, and other push-style stanzas. 3. Keep assertions tolerant of ordering when possible; use `prof_output_regex()` for flexible matching. 4. If timing issues appear, use `prof_timeout()` around critical expectations and reset afterwards. +5. When adding new tests, place them in the appropriate group based on functionality. diff --git a/Makefile.am b/Makefile.am index b8c8c706..89809594 100644 --- a/Makefile.am +++ b/Makefile.am @@ -308,18 +308,22 @@ tests_functionaltests_functionaltests_LDADD = -lcmocka -lstabber @FORKPTY_LIB@ # Parallel functional tests target (~3x faster than sequential) # Usage: make check-functional-parallel +# To add more groups: increase FUNC_TEST_GROUPS and add group in functionaltests.c +FUNC_TEST_GROUPS = 1 2 3 4 + check-functional-parallel: tests/functionaltests/functionaltests - @echo "Running functional tests in parallel (4 groups)..." + @echo "Running functional tests in parallel ($(words $(FUNC_TEST_GROUPS)) groups)..." @mkdir -p $(builddir)/test-logs - @failed=0; \ - ./tests/functionaltests/functionaltests 1 > $(builddir)/test-logs/group1.log 2>&1 & pid1=$$!; \ - ./tests/functionaltests/functionaltests 2 > $(builddir)/test-logs/group2.log 2>&1 & pid2=$$!; \ - ./tests/functionaltests/functionaltests 3 > $(builddir)/test-logs/group3.log 2>&1 & pid3=$$!; \ - ./tests/functionaltests/functionaltests 4 > $(builddir)/test-logs/group4.log 2>&1 & pid4=$$!; \ - wait $$pid1 || { echo "Group 1 FAILED (exit $$?)"; cat $(builddir)/test-logs/group1.log; failed=1; }; \ - wait $$pid2 || { echo "Group 2 FAILED (exit $$?)"; cat $(builddir)/test-logs/group2.log; failed=1; }; \ - wait $$pid3 || { echo "Group 3 FAILED (exit $$?)"; cat $(builddir)/test-logs/group3.log; failed=1; }; \ - wait $$pid4 || { echo "Group 4 FAILED (exit $$?)"; cat $(builddir)/test-logs/group4.log; failed=1; }; \ + @pids=""; \ + for g in $(FUNC_TEST_GROUPS); do \ + ./tests/functionaltests/functionaltests $$g > $(builddir)/test-logs/group$$g.log 2>&1 & \ + pids="$$pids $$!"; \ + done; \ + failed=0; i=1; \ + for pid in $$pids; do \ + wait $$pid || { echo "Group $$i FAILED"; cat $(builddir)/test-logs/group$$i.log; failed=1; }; \ + i=$$((i + 1)); \ + done; \ echo "=== Test Results Summary ==="; \ grep -E 'PASSED|FAILED|Running' $(builddir)/test-logs/group*.log || true; \ if [ $$failed -ne 0 ]; then echo "FUNCTIONAL TESTS FAILED"; exit 1; fi; \ diff --git a/tests/functionaltests/functionaltests.c b/tests/functionaltests/functionaltests.c index cd2a34bc..e5f59d4e 100644 --- a/tests/functionaltests/functionaltests.c +++ b/tests/functionaltests/functionaltests.c @@ -15,17 +15,14 @@ * functional tests run less frequently than unit tests. * * Tests are organized into groups for better maintainability and parallel execution: - * Group 1: Connect, Ping, Rooms, Software (17 tests) - * Group 2: Message, Receipts, Roster, Chat Session (17 tests) - * Group 3: Presence (16 tests) - * Group 4: MUC, Carbons (19 tests) + * Group 1: Connect, Ping, Rooms, Software + * Group 2: Message, Receipts, Roster, Chat Session + * Group 3: Presence, Disconnect + * Group 4: MUC, Carbons * * Parallel execution: * ./functionaltests - run all tests sequentially - * ./functionaltests 1 - run group 1 only - * ./functionaltests 2 - run group 2 only - * ./functionaltests 3 - run group 3 only - * ./functionaltests 4 - run group 4 only + * ./functionaltests N - run group N only (N = 1..num_groups) * * For parallel execution, run multiple groups simultaneously: * ./functionaltests 1 & ./functionaltests 2 & ./functionaltests 3 & ./functionaltests 4 & wait @@ -41,6 +38,7 @@ #include "config.h" +#include "common.h" #include "proftest.h" #include "test_connect.h" #include "test_ping.h" @@ -71,11 +69,12 @@ main(int argc, char* argv[]) } } - /* GROUP 1: Connect, Ping, Rooms, Software (17 tests) - * Basic XMPP session establishment and server queries */ - - /* Connection tests - verify login, roster, bookmarks */ + /* ============================================================ + * GROUP 1: Connect, Ping, Rooms, Software + * Basic XMPP session establishment and server queries + * ============================================================ */ const struct CMUnitTest group1_tests[] = { + /* Connection tests - verify login, roster, bookmarks */ PROF_FUNC_TEST(connect_jid_requests_roster), PROF_FUNC_TEST(connect_jid_sends_presence_after_receiving_roster), PROF_FUNC_TEST(connect_jid_requests_bookmarks), @@ -101,10 +100,11 @@ main(int argc, char* argv[]) PROF_FUNC_TEST(display_software_version_result_in_chat), }; - /* GROUP 2: Message, Receipts, Roster, Chat Session (17 tests) - * Core messaging and contact management */ + /* ============================================================ + * GROUP 2: Message, Receipts, Roster, Chat Session + * Core messaging and contact management + * ============================================================ */ const struct CMUnitTest group2_tests[] = { - /* Basic message send/receive */ PROF_FUNC_TEST(message_send), PROF_FUNC_TEST(message_receive_console), @@ -131,8 +131,10 @@ main(int argc, char* argv[]) PROF_FUNC_TEST(new_session_when_message_received_from_different_fulljid), }; - /* GROUP 3: Presence (16 tests) - * Online/away/xa/dnd/chat status management */ + /* ============================================================ + * GROUP 3: Presence, Disconnect + * Online/away/xa/dnd/chat status management + * ============================================================ */ const struct CMUnitTest group3_tests[] = { PROF_FUNC_TEST(presence_online), PROF_FUNC_TEST(presence_online_with_message), @@ -154,10 +156,11 @@ main(int argc, char* argv[]) PROF_FUNC_TEST(disconnect_ends_session), }; - /* GROUP 4: MUC, Carbons (19 tests) - * Multi-user chat and message synchronization */ + /* ============================================================ + * GROUP 4: MUC, Carbons + * Multi-user chat and message synchronization + * ============================================================ */ const struct CMUnitTest group4_tests[] = { - /* MUC room join with various options - XEP-0045 */ PROF_FUNC_TEST(sends_room_join), PROF_FUNC_TEST(sends_room_join_with_nick), @@ -188,28 +191,31 @@ main(int argc, char* argv[]) PROF_FUNC_TEST(receive_private_carbon), }; + /* Test group registry for easy extension */ + struct { + const char* name; + const struct CMUnitTest* tests; + size_t count; + } groups[] = { + { "Group 1: Connect/Ping/Rooms/Software", group1_tests, ARRAY_SIZE(group1_tests) }, + { "Group 2: Message/Receipts/Roster/Session", group2_tests, ARRAY_SIZE(group2_tests) }, + { "Group 3: Presence/Disconnect", group3_tests, ARRAY_SIZE(group3_tests) }, + { "Group 4: MUC/Carbons", group4_tests, ARRAY_SIZE(group4_tests) }, + }; + const int num_groups = ARRAY_SIZE(groups); + int result = 0; - switch (group) { - case 1: - result = cmocka_run_group_tests_name("Group 1: Connect/Ping/Rooms/Software", group1_tests, NULL, NULL); - break; - case 2: - result = cmocka_run_group_tests_name("Group 2: Message/Receipts/Roster/Session", group2_tests, NULL, NULL); - break; - case 3: - result = cmocka_run_group_tests_name("Group 3: Presence", group3_tests, NULL, NULL); - break; - case 4: - result = cmocka_run_group_tests_name("Group 4: MUC/Carbons", group4_tests, NULL, NULL); - break; - default: - /* Run all groups sequentially */ - result |= cmocka_run_group_tests_name("Group 1: Connect/Ping/Rooms/Software", group1_tests, NULL, NULL); - result |= cmocka_run_group_tests_name("Group 2: Message/Receipts/Roster/Session", group2_tests, NULL, NULL); - result |= cmocka_run_group_tests_name("Group 3: Presence", group3_tests, NULL, NULL); - result |= cmocka_run_group_tests_name("Group 4: MUC/Carbons", group4_tests, NULL, NULL); - break; + if (group > 0 && group <= num_groups) { + /* Run specific group */ + result = _cmocka_run_group_tests(groups[group - 1].name, groups[group - 1].tests, + groups[group - 1].count, NULL, NULL); + } else { + /* Run all groups sequentially */ + for (int i = 0; i < num_groups; i++) { + result |= _cmocka_run_group_tests(groups[i].name, groups[i].tests, + groups[i].count, NULL, NULL); + } } return result; diff --git a/tests/functionaltests/proftest.h b/tests/functionaltests/proftest.h index cafa7056..d8229fce 100644 --- a/tests/functionaltests/proftest.h +++ b/tests/functionaltests/proftest.h @@ -2,9 +2,9 @@ #define __H_PROFTEST /* - * XDG paths are now dynamic, generated per-test based on stub_port. - * This allows parallel test execution without file conflicts. - * Use xdg_config_home and xdg_data_home variables instead of macros. + * XDG paths are dynamic and generated per-test based on stub_port. + * Each test instance uses unique directories (./tests/functionaltests/files/{port}/xdg_*) + * to allow parallel test execution without file conflicts. */ extern char xdg_config_home[256]; extern char xdg_data_home[256];