Improve Functional Test Stability and CI Reliability #79

Manually merged
jabber.developer merged 7 commits from fix/test-CI-stability into master 2026-02-02 16:47:30 +00:00
Collaborator

Description

This PR significantly improves CI reliability, test stability, and code quality by reorganizing the build pipeline and adding comprehensive verification mechanisms.

Changes Made

CI Pipeline Improvements:

  • Refactored ci-build.sh to run 4 parallel build configurations with different feature flags
  • Added test failure detection verification (meta-test that ensures test framework catches failures)
  • Added --coverage-only mode for dedicated coverage job
  • Integrated Valgrind checks for all Linux builds
  • Proper exit code handling for configure, make, and tests
  • Detailed per-build statistics output (unit/functional tests, coverage, duration)
  • Added constants for regex patterns and helper functions for better maintainability
  • Updated documentation in CONTRIBUTING.md

Coverage Collection:

  • Separated coverage job from regular linux builds (faster CI)
  • Fixed coverage calculation to exclude test files (only production code from src)

Test Infrastructure:

  • Fixed parallel test execution conflicts with PROF_BUILD_INDEX port allocation
  • Added cleanup targets in Makefile.am
  • Updated .gitignore for test artifacts and coverage files
  • Added Valgrind suppressions in prof.supp

Testing

  • All 4 build configurations pass
  • Manual error injection tests (unit or/and func or Valgrind) verified CI correctly detects and reports failures
  • Coverage job correctly reports ~27% line coverage
  • Valgrind finds no memory leaks in unit tests
  • Parallel functional tests don't conflict

Closes #78 - CI improvements for pull request testing.
Issue was introduced in #75

### Description This PR significantly improves CI reliability, test stability, and code quality by reorganizing the build pipeline and adding comprehensive verification mechanisms. ### Changes Made **CI Pipeline Improvements:** - Refactored ci-build.sh to run 4 parallel build configurations with different feature flags - Added test failure detection verification (meta-test that ensures test framework catches failures) - Added `--coverage-only` mode for dedicated coverage job - Integrated Valgrind checks for all Linux builds - Proper exit code handling for configure, make, and tests - Detailed per-build statistics output (unit/functional tests, coverage, duration) - Added constants for regex patterns and helper functions for better maintainability - Updated documentation in CONTRIBUTING.md **Coverage Collection:** - Separated coverage job from regular linux builds (faster CI) - Fixed coverage calculation to exclude test files (only production code from src) **Test Infrastructure:** - Fixed parallel test execution conflicts with `PROF_BUILD_INDEX` port allocation - Added cleanup targets in Makefile.am - Updated .gitignore for test artifacts and coverage files - Added Valgrind suppressions in prof.supp ### Testing - All 4 build configurations pass - Manual error injection tests (unit or/and func or Valgrind) verified CI correctly detects and reports failures - Coverage job correctly reports ~27% line coverage - Valgrind finds no memory leaks in unit tests - Parallel functional tests don't conflict ### Related Issues Closes #78 - CI improvements for pull request testing. Issue was introduced in #75
jabber.developer2 added 2 commits 2026-01-22 13:58:31 +00:00
fix: improve functional test stability and CI reliability
All checks were successful
CI Code / Check spelling (pull_request) Successful in 18s
CI Code / Check coding style (pull_request) Successful in 34s
CI Code / Code Coverage (pull_request) Successful in 15m26s
CI Code / Linux (ubuntu) (pull_request) Successful in 18m15s
CI Code / Linux (debian) (pull_request) Successful in 18m27s
CI Code / Linux (arch) (pull_request) Successful in 21m9s
c573cdb26d
- Fix parallel test execution conflicts by moving test files to build dir
- Improve error handling in ci-build.sh with proper exit codes
- Update test file paths for out-of-tree build compatibility
- Add test-files and test-logs to .gitignore
- Add clean-functional-tests target to Makefile.am
jabber.developer2 force-pushed fix/test-CI-stability from c573cdb26d to 2fb9b3fed9 2026-01-28 16:45:53 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from 2fb9b3fed9 to 01c100a60b 2026-01-28 16:48:36 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from 01c100a60b to 52a805d1ce 2026-01-28 17:01:09 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from 52a805d1ce to ce4cb13470 2026-01-28 17:21:22 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from ce4cb13470 to 9a2a7614a9 2026-01-28 17:42:45 +00:00 Compare
jabber.developer2 added 1 commit 2026-01-28 19:52:12 +00:00
ci: fix coverage to match Code Coverage job output
All checks were successful
CI Code / Check spelling (pull_request) Successful in 21s
CI Code / Check coding style (pull_request) Successful in 37s
CI Code / Linux (debian) (pull_request) Successful in 6m40s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m39s
CI Code / Linux (arch) (pull_request) Successful in 6m50s
CI Code / Code Coverage (pull_request) Successful in 15m25s
f0c9417471
Use --extract '*/src/src/*' instead of --remove to properly filter
production code from tests. Coverage now shows 27.5% lines matching
the dedicated Code Coverage job.
jabber.developer2 force-pushed fix/test-CI-stability from f0c9417471 to 6c5c523a1a 2026-01-28 20:10:25 +00:00 Compare
jabber.developer2 added 1 commit 2026-01-29 11:58:00 +00:00
test: add intentional test failures to verify CI detection
Some checks failed
CI Code / Check spelling (pull_request) Successful in 22s
CI Code / Check coding style (pull_request) Successful in 36s
CI Code / Linux (debian) (pull_request) Failing after 3m5s
CI Code / Linux (ubuntu) (pull_request) Failing after 3m6s
CI Code / Linux (arch) (pull_request) Failing after 3m12s
CI Code / Code Coverage (pull_request) Successful in 15m42s
ef673c2527
jabber.developer2 force-pushed fix/test-CI-stability from ef673c2527 to d3526007d0 2026-01-29 12:10:48 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from d3526007d0 to 35f1902cbc 2026-01-29 12:14:20 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from 35f1902cbc to dc9aed00a6 2026-01-29 12:15:44 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from dc9aed00a6 to e5330ab0a8 2026-01-29 12:17:49 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from e5330ab0a8 to 6c5c523a1a 2026-01-29 12:25:09 +00:00 Compare
jabber.developer2 added 1 commit 2026-01-29 12:53:17 +00:00
ci: integrate coverage into linux arch job, remove duplicate
All checks were successful
CI Code / Check spelling (pull_request) Successful in 19s
CI Code / Check coding style (pull_request) Successful in 33s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m58s
CI Code / Linux (debian) (pull_request) Successful in 7m3s
CI Code / Linux (arch) (pull_request) Successful in 7m6s
2c308e824e
- Linux arch job now uploads coverage.info as artifact
- ci-build.sh saves coverage.info before cleanup
- Removed separate coverage job (was duplicating arch build)
- Saves ~25% CI time by eliminating redundant build
jabber.developer2 force-pushed fix/test-CI-stability from 2c308e824e to 7795877306 2026-01-29 13:06:15 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from 7795877306 to 42c04d28fc 2026-01-29 13:51:42 +00:00 Compare
jabber.developer2 force-pushed fix/test-CI-stability from 42c04d28fc to adc5e26689 2026-01-29 14:01:43 +00:00 Compare
jabber.developer2 added 1 commit 2026-01-29 14:08:52 +00:00
ci: remove coverage from linux builds
All checks were successful
CI Code / Check spelling (pull_request) Successful in 25s
CI Code / Check coding style (pull_request) Successful in 33s
CI Code / Code Coverage (pull_request) Successful in 4m46s
CI Code / Linux (debian) (pull_request) Successful in 6m7s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m13s
CI Code / Linux (arch) (pull_request) Successful in 6m21s
5100efca3b
Coverage is now collected only in the dedicated Code Coverage job.
jabber.developer2 added 1 commit 2026-01-29 14:45:14 +00:00
ci: refactor ci-build.sh with constants and helper functions
All checks were successful
CI Code / Check spelling (pull_request) Successful in 21s
CI Code / Check coding style (pull_request) Successful in 33s
CI Code / Code Coverage (pull_request) Successful in 4m46s
CI Code / Linux (debian) (pull_request) Successful in 6m8s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m14s
CI Code / Linux (arch) (pull_request) Successful in 6m19s
cf86e60af6
- Add CMOCKA_PASSED_PATTERN, CMOCKA_FAILED_PATTERN constants
- Add COVERAGE_PATTERNS constant for lcov extraction
- Add parse_build_stats() function for parsing build logs
- Add extract_test_count() helper function
- Organize code into sections: Constants, Helper Functions, Test Verification
jabber.developer2 changed title from WIP: Improve Functional Test Stability and CI Reliability to Improve Functional Test Stability and CI Reliability 2026-01-29 14:54:14 +00:00
jabber.developer requested changes 2026-01-30 18:59:15 +00:00
Dismissed
jabber.developer left a comment
Owner

Thank you for the PR. It's a great functional improvement. On the other note, I'd like to see slightly better practices in terms of readability and general coding best practices (avoiding magic numbers, commentary that add context rather than repeat code, and, importantly, unified error handling stategy). Nonetheless, solid PR overall.

Thank you for the PR. It's a great functional improvement. On the other note, I'd like to see slightly better practices in terms of readability and general coding best practices (avoiding magic numbers, commentary that add context rather than repeat code, and, importantly, unified error handling stategy). Nonetheless, solid PR overall.
CONTRIBUTING.md Outdated
@@ -91,2 +91,4 @@
This will run the same tests that the CI runs and refuse the push if it fails.
The CI script runs 4 parallel builds with different configurations, each with Valgrind and functional tests.
Use `./ci-build.sh --coverage-only` to run only build 1 with coverage collection.

Could you clarify that "build 1" is a standard build without flags?

Could you clarify that "build 1" is a standard build without flags?
Author
Collaborator

Corrected

Corrected
jabber.developer marked this conversation as resolved
@@ -26,0 +40,4 @@
# Parse STATS line from build log and set global variables
# Usage: parse_build_stats <log_file>
# Sets: STAT_UNIT_P, STAT_UNIT_F, STAT_FUNC_P, STAT_FUNC_F,

Is it required? It needs to be updated each time the set variables of the method are updated, yet it does not provide additional data. It would make more sense to explain what each variable means next to their initiation place.

Is it required? It needs to be updated each time the set variables of the method are updated, yet it does not provide additional data. It would make more sense to explain what each variable means next to their initiation place.
Author
Collaborator

Refactored, moved to separate function

Refactored, moved to separate function
jabber.developer marked this conversation as resolved
ci-build.sh Outdated
@@ -216,3 +261,3 @@
# shellcheck disable=SC2086
../configure -C $features $extra_args
if ! ../configure -C $features $extra_args; then

Wouldn't it get caught in the error handler declared earlier?

Wouldn't it get caught in the error handler declared earlier?
Author
Collaborator

It is done foe log readability.

It is done foe log readability.
jabber.developer marked this conversation as resolved
ci-build.sh Outdated
@@ -219,3 +267,2 @@
$MAKE CC="${CC}"
$MAKE check-functional-parallel
if ! $MAKE CC="${CC}"; then

(same as above) Wouldn't it get caught in the error handler declared earlier?

(same as above) Wouldn't it get caught in the error handler declared earlier?
Author
Collaborator

It is done foe log readability.

It is done foe log readability.
jabber.developer marked this conversation as resolved
ci-build.sh Outdated
@@ -224,0 +356,4 @@
# Save coverage.info before cleanup (for CI artifact)
if [ "$run_coverage" = "yes" ] && [ -f coverage.info ]; then
cp coverage.info ../coverage.info

Wouldn't it create trash in non-CI (local) run?

Wouldn't it create trash in non-CI (local) run?
Author
Collaborator

Added [ -n "$CI" ] check. Artifacts only created in CI environment now.

Added [ -n "$CI" ] check. Artifacts only created in CI environment now.
jabber.developer marked this conversation as resolved
ci-build.sh Outdated
@@ -243,0 +400,4 @@
run_valgrind="yes"
extra_flags="--enable-valgrind"
else
extra_flags=""

Can we repeat order of the variables for readability? I.e. run_valgrind and then extra_flags to match order above

Can we repeat order of the variables for readability? I.e. `run_valgrind` and then `extra_flags` to match order above
Author
Collaborator

Unified

Unified
jabber.developer marked this conversation as resolved
@@ -138,1 +138,4 @@
{
const char *group_env = getenv("PROF_TEST_GROUP");
int group = group_env ? atoi(group_env) : 0;
int dir_id = (group >= 1 && group <= 4) ? group : stub_port;

IMPORTANT:
Here and everywhere else: can we avoid magic constant of "4"? Can we make "TEST_GROUPS" constant or something similar? This way it would be much easier to adjust in the future

IMPORTANT: Here and everywhere else: can we avoid magic constant of "4"? Can we make "TEST_GROUPS" constant or something similar? This way it would be much easier to adjust in the future
Author
Collaborator

Added #define TEST_GROUPS 4 constant.

Added #define TEST_GROUPS 4 constant.
jabber.developer marked this conversation as resolved
@@ -229,0 +245,4 @@
/* Calculate port base: each build uses a different range of 4 ports
* Build 0/1: 5230-5233, Build 2: 5234-5237, Build 3: 5238-5241, Build 4: 5242-5245 */
int port_base = 5230 + ((build_idx > 0 ? build_idx - 1 : 0) * 4);

Why 2 builds (0 and 1) share the same ports range?

Why 2 builds (0 and 1) share the same ports range?
Author
Collaborator

Added explanation: builds 0/1 share ports because they're mutually exclusive (sequential mode or parallel).

Added explanation: builds 0/1 share ports because they're mutually exclusive (sequential mode or parallel).
jabber.developer marked this conversation as resolved
@@ -234,0 +255,4 @@
if (group >= 1 && group <= 4) {
/* Static allocation: each group gets a dedicated port */
stub_port = port_base + group - 1;
fprintf(stderr, "[PROF_TEST] Build %d, Group %d: trying port %d\n", build_idx, group, stub_port);

It shouldn't be at the stderr. It would make more sense to put it in the stdout, since it's informational/debug data.

It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
Author
Collaborator

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
jabber.developer marked this conversation as resolved
@@ -234,2 +260,3 @@
if (stbbr_start(STBBR_LOGDEBUG, stub_port, 0) == 0) {
started = TRUE;
break;
fprintf(stderr, "[PROF_TEST] Started stabber on port %d\n", stub_port);

It shouldn't be at the stderr. It would make more sense to put it in the stdout, since it's informational/debug data.

It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
Author
Collaborator

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
jabber.developer marked this conversation as resolved
@@ -238,2 +268,3 @@
/* Fallback to dynamic allocation if static failed or group=0 */
if (!started) {
assert_true(FALSE); // could not start stabber on any port in range
fprintf(stderr, "[PROF_TEST] Using dynamic port allocation\n");

It shouldn't be at the stderr. It would make more sense to put it in the stdout, since it's informational/debug data.

It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
Author
Collaborator

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
jabber.developer marked this conversation as resolved
@@ -240,0 +272,4 @@
if (stbbr_start(STBBR_LOGDEBUG, p, 0) == 0) {
stub_port = p;
started = TRUE;
fprintf(stderr, "[PROF_TEST] Started stabber on port %d\n", stub_port);

It shouldn't be at the stderr. It would make more sense to put it in the stdout, since it's informational/debug data.

It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
Author
Collaborator

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.

Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
jabber.developer marked this conversation as resolved
@@ -240,0 +280,4 @@
if (!started) {
fprintf(stderr, "[PROF_TEST] ERROR: could not start stabber on any port\n");
assert_true(FALSE);

Why do we need to do this?

Why do we need to do this?
Author
Collaborator

Removed.

Removed.
jabber.developer marked this conversation as resolved
@@ -247,1 +293,3 @@
"./tests/functionaltests/files/%d/xdg_data_home", stub_port);
"./test-files/%d/xdg_data_home", dir_id);
fprintf(stderr, "[PROF_TEST] Group %d using directories: config=%s, data=%s\n",

same as above

same as above
Author
Collaborator

Removed

Removed
jabber.developer marked this conversation as resolved
@@ -399,3 +448,3 @@
}
usleep(50000); /* 50ms */
usleep(50000);

50 ms clarifying comment was on point. But it should be formatted in the same manner as other comments (for inline comment, use either /* or //, not both)

50 ms clarifying comment was on point. But it should be formatted in the same manner as other comments (for inline comment, use either `/*` or `//`, not both)
Author
Collaborator

corrected

corrected
jabber.developer2 added 1 commit 2026-02-02 11:57:22 +00:00
fix: address code review comments from PR #79
All checks were successful
CI Code / Check spelling (pull_request) Successful in 17s
CI Code / Check coding style (pull_request) Successful in 37s
CI Code / Code Coverage (pull_request) Successful in 4m47s
CI Code / Linux (debian) (pull_request) Successful in 6m8s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m12s
CI Code / Linux (arch) (pull_request) Successful in 6m19s
5978c77a45
- Add TEST_GROUPS constant to eliminate magic number
- Use stdout for info messages, stderr for errors only
- Remove redundant comments and dead code
- Add CI check before copying coverage.info
- Unify variable declaration order
- Add port allocation explanation comment
- Restore /* 50ms */ inline comment
- Update CONTRIBUTING.md with build 1 description
Author
Collaborator

suggested squash commit message
ci: improve CI stability with parallel builds and Valgrind

Major changes:

  • Run 4 build configurations in parallel with Valgrind on Linux
  • Add test failure detection verification (meta-test)
  • Port allocation per build to prevent conflicts in parallel runs
  • Add --coverage-only flag for dedicated coverage builds

Code quality:

  • Add TEST_GROUPS constant, CMOCKA patterns, helper functions
  • Organize ci-build.sh into sections
suggested squash commit message ci: improve CI stability with parallel builds and Valgrind Major changes: - Run 4 build configurations in parallel with Valgrind on Linux - Add test failure detection verification (meta-test) - Port allocation per build to prevent conflicts in parallel runs - Add --coverage-only flag for dedicated coverage builds Code quality: - Add TEST_GROUPS constant, CMOCKA patterns, helper functions - Organize ci-build.sh into sections
jabber.developer requested changes 2026-02-02 12:42:25 +00:00
Dismissed
jabber.developer left a comment
Owner

Generally LGTM, but errors should go to stderr. Ideally, we should also change 1 2 3 4 to use a constant (something like 1 ... TEST_GROUPS). Please fix these minor issues and I can merge it ASAP.

Generally LGTM, but errors should go to `stderr`. Ideally, we should also change `1 2 3 4` to use a constant (something like `1 ... TEST_GROUPS`). Please fix these minor issues and I can merge it ASAP.
@@ -229,0 +249,4 @@
/* Calculate port base: each build uses a different range of TEST_GROUPS ports.
* Build 0 (local/default): 5230-5233, Build 1: 5230-5233, Build 2: 5234-5237, etc.
* Build 0 and 1 share the same range because build 0 is for local runs (no parallel builds),
* while builds 1-4 are used in CI where they run in parallel. */

Can we do something with the magic number 4 in the comments as well? Here and elsewhere

Can we do something with the magic number 4 in the comments as well? Here and elsewhere
Author
Collaborator

Config names added additional to predefined values.

Config names added additional to predefined values.
jabber.developer marked this conversation as resolved
@@ -235,1 +267,3 @@
break;
printf("[PROF_TEST] Started stabber on port %d\n", stub_port);
} else {
printf("[PROF_TEST] Failed to start stabber on port %d\n", stub_port);

Should be in stderr. Please check analogous cases

Should be in stderr. Please check analogous cases
Author
Collaborator

Corrected.

Corrected.
jabber.developer marked this conversation as resolved
jabber.developer2 added 1 commit 2026-02-02 13:04:19 +00:00
fix: add build names, use TEST_BUILDS constant, redirect errors to stderr
All checks were successful
CI Code / Check spelling (pull_request) Successful in 19s
CI Code / Check coding style (pull_request) Successful in 32s
CI Code / Code Coverage (pull_request) Successful in 4m45s
CI Code / Linux (debian) (pull_request) Successful in 6m8s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m15s
CI Code / Linux (arch) (pull_request) Successful in 6m21s
a546092a5f
jabber.developer requested review from jabber.developer 2026-02-02 16:42:02 +00:00
jabber.developer approved these changes 2026-02-02 16:42:29 +00:00
jabber.developer left a comment
Owner

LGTM

LGTM
jabber.developer manually merged commit f8826b7c79 into master 2026-02-02 16:47:30 +00:00
Sign in to join this conversation.
No description provided.