Improve Functional Test Stability and CI Reliability #79
@@ -91,7 +91,7 @@ set -e
|
||||
|
||||
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.
|
||||
Use `./ci-build.sh --coverage-only` to run only build 1 (full configuration with all optional components enabled) with coverage collection.
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
|
||||
|
||||
Output shows test results per build:
|
||||
```
|
||||
|
||||
@@ -40,8 +40,6 @@ readonly COVERAGE_PATTERNS='*/profanity/src/* */src/src/*'
|
||||
|
||||
# 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,
|
||||
# STAT_COV_LINES, STAT_COV_FUNCS, STAT_COV_BRANCHES, STAT_TIME
|
||||
parse_build_stats() {
|
||||
|
jabber.developer marked this conversation as resolved
jabber.developer
commented
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.
jabber.developer2
commented
Refactored, moved to separate function Refactored, moved to separate function
|
||||
local log_file="$1"
|
||||
local stats_line
|
||||
@@ -355,7 +353,8 @@ build_and_test() {
|
||||
./profanity -v
|
||||
|
||||
# Save coverage.info before cleanup (for CI artifact)
|
||||
if [ "$run_coverage" = "yes" ] && [ -f coverage.info ]; then
|
||||
# Only copy in CI environment to avoid leaving artifacts during local runs
|
||||
if [ "$run_coverage" = "yes" ] && [ -f coverage.info ] && [ -n "$CI" ]; then
|
||||
cp coverage.info ../coverage.info
|
||||
echo "Coverage report saved to coverage.info"
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
jabber.developer
commented
Wouldn't it create trash in non-CI (local) run? Wouldn't it create trash in non-CI (local) run?
jabber.developer2
commented
Added [ -n "$CI" ] check. Artifacts only created in CI environment now. Added [ -n "$CI" ] check. Artifacts only created in CI environment now.
|
||||
fi
|
||||
@@ -397,8 +396,8 @@ else
|
||||
if [ $idx -le ${#tests[@]} ]; then
|
||||
# All builds run Valgrind on Linux
|
||||
if [ "$ARCH" = "linux" ]; then
|
||||
run_valgrind="yes"
|
||||
extra_flags="--enable-valgrind"
|
||||
run_valgrind="yes"
|
||||
else
|
||||
extra_flags=""
|
||||
run_valgrind="no"
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
jabber.developer
commented
Can we repeat order of the variables for readability? I.e. Can we repeat order of the variables for readability? I.e. `run_valgrind` and then `extra_flags` to match order above
jabber.developer2
commented
Unified Unified
|
||||
|
||||
@@ -17,6 +17,9 @@
|
||||
|
||||
#include "proftest.h"
|
||||
|
||||
/* Number of parallel test groups for CI builds */
|
||||
#define TEST_GROUPS 4
|
||||
|
||||
char *config_orig;
|
||||
char *data_orig;
|
||||
|
||||
@@ -138,9 +141,9 @@ _cleanup_dirs(void)
|
||||
{
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
jabber.developer
commented
IMPORTANT: 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
jabber.developer2
commented
Added #define TEST_GROUPS 4 constant. Added #define TEST_GROUPS 4 constant.
|
||||
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;
|
||||
int dir_id = (group >= 1 && group <= TEST_GROUPS) ? group : stub_port;
|
||||
|
||||
fprintf(stderr, "[PROF_TEST] Cleaning up directories for group %d (dir_id %d)\n", group, dir_id);
|
||||
printf("[PROF_TEST] Cleaning up directories for group %d (dir_id %d)\n", group, dir_id);
|
||||
|
||||
char cmd[512];
|
||||
snprintf(cmd, sizeof(cmd), "rm -rf ./test-files/%d", dir_id);
|
||||
@@ -243,36 +246,38 @@ init_prof_test(void **state)
|
||||
const char *build_env = getenv("PROF_BUILD_INDEX");
|
||||
int build_idx = build_env ? atoi(build_env) : 0;
|
||||
|
||||
|
jabber.developer marked this conversation as resolved
jabber.developer
commented
Why 2 builds (0 and 1) share the same ports range? Why 2 builds (0 and 1) share the same ports range?
jabber.developer2
commented
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).
|
||||
/* 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);
|
||||
/* 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. */
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
jabber.developer
commented
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
jabber.developer2
commented
Config names added additional to predefined values. Config names added additional to predefined values.
|
||||
int port_base = 5230 + ((build_idx > 0 ? build_idx - 1 : 0) * TEST_GROUPS);
|
||||
|
||||
/* Static resource allocation to avoid conflicts in parallel execution.
|
||||
* Group 1-4: use static port assignment.
|
||||
* Group 0 (all groups): use dynamic allocation as fallback. */
|
||||
gboolean started = FALSE;
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
jabber.developer
commented
It shouldn't be at the It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
jabber.developer2
commented
Fixed. Info messages now use printf() (stdout), stderr reserved for errors only. Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
|
||||
|
||||
if (group >= 1 && group <= 4) {
|
||||
if (group >= 1 && group <= TEST_GROUPS) {
|
||||
/* Static allocation: each group gets a dedicated port */
|
||||
stub_port = port_base + group - 1;
|
||||
|
jabber.developer marked this conversation as resolved
jabber.developer
commented
It shouldn't be at the It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
jabber.developer2
commented
Fixed. Info messages now use printf() (stdout), stderr reserved for errors only. Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
|
||||
fprintf(stderr, "[PROF_TEST] Build %d, Group %d: trying port %d\n", build_idx, group, stub_port);
|
||||
printf("[PROF_TEST] Build %d, Group %d: trying port %d\n", build_idx, group, stub_port);
|
||||
|
||||
if (stbbr_start(STBBR_LOGDEBUG, stub_port, 0) == 0) {
|
||||
started = TRUE;
|
||||
fprintf(stderr, "[PROF_TEST] Started stabber on port %d\n", stub_port);
|
||||
printf("[PROF_TEST] Started stabber on port %d\n", stub_port);
|
||||
} else {
|
||||
fprintf(stderr, "[PROF_TEST] Failed to start stabber on port %d\n", stub_port);
|
||||
printf("[PROF_TEST] Failed to start stabber on port %d\n", stub_port);
|
||||
|
jabber.developer marked this conversation as resolved
jabber.developer
commented
Should be in stderr. Please check analogous cases Should be in stderr. Please check analogous cases
jabber.developer2
commented
Corrected. Corrected.
|
||||
}
|
||||
|
jabber.developer marked this conversation as resolved
jabber.developer
commented
It shouldn't be at the It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
jabber.developer2
commented
Fixed. Info messages now use printf() (stdout), stderr reserved for errors only. Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
|
||||
}
|
||||
|
||||
/* Fallback to dynamic allocation if static failed or group=0 */
|
||||
if (!started) {
|
||||
fprintf(stderr, "[PROF_TEST] Using dynamic port allocation\n");
|
||||
printf("[PROF_TEST] Using dynamic port allocation\n");
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
jabber.developer
commented
It shouldn't be at the It shouldn't be at the `stderr`. It would make more sense to put it in the stdout, since it's informational/debug data.
jabber.developer2
commented
Fixed. Info messages now use printf() (stdout), stderr reserved for errors only. Fixed. Info messages now use printf() (stdout), stderr reserved for errors only.
|
||||
for (int p = port_base; p < port_base + 20; ++p) {
|
||||
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);
|
||||
printf("[PROF_TEST] Started stabber on port %d\n", stub_port);
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
jabber.developer marked this conversation as resolved
Outdated
jabber.developer
commented
Why do we need to do this? Why do we need to do this?
jabber.developer2
commented
Removed. Removed.
|
||||
@@ -280,20 +285,19 @@ init_prof_test(void **state)
|
||||
|
||||
if (!started) {
|
||||
fprintf(stderr, "[PROF_TEST] ERROR: could not start stabber on any port\n");
|
||||
assert_true(FALSE);
|
||||
return -1;
|
||||
}
|
||||
|
||||
// Generate unique XDG paths based on group for parallel execution
|
||||
// Use ./test-files/ in current (build) directory for out-of-tree builds compatibility
|
||||
int dir_id = (group >= 1 && group <= 4) ? group : stub_port;
|
||||
/* Generate unique XDG paths based on group for parallel execution.
|
||||
* Use ./test-files/ in current (build) directory for out-of-tree builds compatibility. */
|
||||
int dir_id = (group >= 1 && group <= TEST_GROUPS) ? group : stub_port;
|
||||
snprintf(xdg_config_home, sizeof(xdg_config_home),
|
||||
"./test-files/%d/xdg_config_home", dir_id);
|
||||
|
jabber.developer marked this conversation as resolved
jabber.developer
commented
same as above same as above
jabber.developer2
commented
Removed Removed
|
||||
snprintf(xdg_data_home, sizeof(xdg_data_home),
|
||||
"./test-files/%d/xdg_data_home", dir_id);
|
||||
|
||||
fprintf(stderr, "[PROF_TEST] Group %d using directories: config=%s, data=%s\n",
|
||||
group, xdg_config_home, xdg_data_home);
|
||||
printf("[PROF_TEST] Group %d using directories: config=%s, data=%s\n",
|
||||
group, xdg_config_home, xdg_data_home);
|
||||
|
||||
// Give stabber server thread time to start listening
|
||||
usleep(100000); // 100ms
|
||||
@@ -447,7 +451,7 @@ prof_output_regex(const char *pattern)
|
||||
return 1;
|
||||
}
|
||||
|
||||
usleep(50000);
|
||||
usleep(50000); /* 50ms */
|
||||
}
|
||||
|
||||
/* Timeout reached - log diagnostic info */
|
||||
|
||||
Could you clarify that "build 1" is a standard build without flags?
Corrected