Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .github/workflows/testing.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ jobs:
runs-on: ubuntu-24.04
steps:
- name: Checkout sources
uses: actions/checkout@v2
uses: actions/checkout@v4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Consider upgrading to v6 for the latest features and security updates.

While upgrading from v2 to v4 is an improvement, v6.0.1 is the latest version. Recommend updating to actions/checkout@v6 to align with current best practices.

🤖 Prompt for AI Agents
In .github/workflows/testing.yml around line 15 the workflow uses
actions/checkout@v4; update this to actions/checkout@v6 (or
actions/checkout@v6.0.1) to pick up the latest features and security fixes, then
run the workflow locally or in CI to verify there are no breaking changes and
adjust any checkout-related inputs if needed.

with:
submodules: 'recursive'
- name: Build
run: make
- name: Test
run: make test
- name: Unit Tests
run: make test-unit
- name: Integration Tests
run: make test-integration

2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
src/*.o
dwgsim*
tests/*.o
tests/run_tests
26 changes: 23 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ all-recur lib-recur clean-recur cleanlocal-recur install-recur:

all:$(PROG)

.PHONY:all lib clean cleanlocal
.PHONY:all lib clean cleanlocal test test-unit test-integration clean-tests
.PHONY:all-recur lib-recur clean-recur cleanlocal-recur install-recur

dwgsim:lib-recur $(DWGSIM_AOBJS)
Expand All @@ -54,7 +54,7 @@ cleanlocal:
fi; \
done;

clean:cleanlocal-recur
clean:cleanlocal-recur clean-tests

dist:clean
if [ -f dwgsim-${PACKAGE_VERSION}.tar.gz ]; then \
Expand All @@ -72,6 +72,26 @@ dist:clean
gzip -9 dwgsim-${PACKAGE_VERSION}.tar; \
rm -rv dwgsim-${PACKAGE_VERSION};

test:
# Run all tests (unit + integration)
test: test-unit test-integration

# Integration tests
test-integration:
if [ -d tmp ]; then rm -r tmp; fi
/bin/bash testdata/test.sh

# Unit test target
TEST_OBJS = tests/test_main.o
TEST_PROG = tests/run_tests

tests/test_main.o: tests/test_main.c tests/test_framework.h
$(CC) -c $(CFLAGS) $(DFLAGS) -I. -Isrc tests/test_main.c -o $@

$(TEST_PROG): $(TEST_OBJS)
$(CC) $(CFLAGS) -o $@ $(TEST_OBJS) -lm

test-unit: $(TEST_PROG)
./$(TEST_PROG)

clean-tests:
rm -f tests/*.o $(TEST_PROG)
224 changes: 224 additions & 0 deletions tests/test_framework.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
/*
* Minimal Unit Test Framework for DWGSIM
*
* Usage:
* #include "test_framework.h"
*
* TEST(test_name) {
* ASSERT(condition);
* ASSERT_EQ(expected, actual);
* ASSERT_STR_EQ(expected, actual);
* }
*
* int main(void) {
* RUN_TEST(test_name);
* TEST_SUMMARY();
* return test_failures;
* }
*/

#ifndef TEST_FRAMEWORK_H
#define TEST_FRAMEWORK_H

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <math.h>

/* Global test counters */
static int test_count = 0;
static int test_failures = 0;
static int test_assertions = 0;
static const char *current_test = NULL;
Comment on lines +28 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Static globals in header limit scalability to single translation unit.

Defining static global variables in a header file means each .c file that includes this header gets its own independent copy of these counters. This design only works correctly if the header is included in exactly one .c file.

Impact:

  • If future test files include this header separately (as proper .c files), their test counts won't aggregate
  • The framework cannot track results across multiple test compilation units

Current status: This works now because only test_main.c includes the header, but it will break if the codebase scales to multiple test files compiled separately.

Recommended solutions:

  1. Move these definitions to test_main.c and declare them extern in the header
  2. Use a single-file pattern where all tests are included as .c files (as hinted in test_main.c comments), though this has other downsides

Consider documenting this limitation in the header comment to warn future contributors.

🤖 Prompt for AI Agents
In tests/test_framework.h around lines 28-32, the static global counters are
defined in the header which creates per-translation-unit copies; change the
header to declare these as extern (e.g. extern int test_count; extern int
test_failures; extern int test_assertions; extern const char *current_test;) and
move the actual definitions (without extern and without static) into test_main.c
(or another single implementation file) so there is one shared instance;
additionally update any include patterns or add a short header comment
documenting that the real definitions live in test_main.c.


/* Test function signature */
typedef void (*test_fn)(void);

/* Define a test function */
#define TEST(name) static void name(void)

/* Run a test */
#define RUN_TEST(name) do { \
current_test = #name; \
test_count++; \
int prev_failures = test_failures; \
name(); \
if (test_failures == prev_failures) { \
printf(" [PASS] %s\n", #name); \
} \
} while (0)

/* Print test summary */
#define TEST_SUMMARY() do { \
printf("\n----------------------------------------\n"); \
printf("Tests: %d | Passed: %d | Failed: %d | Assertions: %d\n", \
test_count, test_count - test_failures, test_failures, test_assertions); \
printf("----------------------------------------\n"); \
} while (0)

/* Basic assertion */
#define ASSERT(cond) do { \
test_assertions++; \
if (!(cond)) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Assertion failed: %s\n", #cond); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert with message */
#define ASSERT_MSG(cond, msg) do { \
test_assertions++; \
if (!(cond)) { \
printf(" [FAIL] %s\n", current_test); \
printf(" %s\n", msg); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert equality for integers */
#define ASSERT_EQ(expected, actual) do { \
test_assertions++; \
long long _exp = (long long)(expected); \
long long _act = (long long)(actual); \
if (_exp != _act) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected: %lld, Actual: %lld\n", _exp, _act); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert inequality */
#define ASSERT_NE(not_expected, actual) do { \
test_assertions++; \
long long _nexp = (long long)(not_expected); \
long long _act = (long long)(actual); \
if (_nexp == _act) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected NOT: %lld, but got: %lld\n", _nexp, _act); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert string equality */
#define ASSERT_STR_EQ(expected, actual) do { \
test_assertions++; \
const char *_exp = (expected); \
const char *_act = (actual); \
if (_exp == NULL || _act == NULL || strcmp(_exp, _act) != 0) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected: \"%s\", Actual: \"%s\"\n", \
_exp ? _exp : "(null)", _act ? _act : "(null)"); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)
Comment on lines +111 to +124
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reconsider NULL handling in ASSERT_STR_EQ.

The current logic treats NULL == NULL as unequal, which may surprise users:

ASSERT_STR_EQ(NULL, NULL);  // This would FAIL

The condition if (_exp == NULL || _act == NULL || strcmp(...)) fails when either pointer is NULL, even if both are NULL.

Most test frameworks allow NULL == NULL to pass. Consider revising to:

if ((_exp == NULL) != (_act == NULL) || (_exp != NULL && strcmp(_exp, _act) != 0))

This would pass when both are NULL, but fail when only one is NULL or when non-NULL strings differ.

🔎 Proposed fix
-    if (_exp == NULL || _act == NULL || strcmp(_exp, _act) != 0) {
+    if ((_exp == NULL) != (_act == NULL) || (_exp != NULL && strcmp(_exp, _act) != 0)) {
         printf("  [FAIL] %s\n", current_test);
         printf("         Expected: \"%s\", Actual: \"%s\"\n", \
                _exp ? _exp : "(null)", _act ? _act : "(null)");
🤖 Prompt for AI Agents
In tests/test_framework.h around lines 111 to 124, the ASSERT_STR_EQ macro
treats NULL == NULL as a failure because it unconditionally fails when either
pointer is NULL; change the boolean condition to first detect a nullity mismatch
or, only when both are non-NULL, call strcmp. Specifically, replace the current
if-check with logic equivalent to: fail if exactly one of _exp/_act is NULL, or
if both non-NULL and strcmp reports inequality; keep the existing printing
behavior (using "(null)" when appropriate) and return on failure.


/* Assert floating point equality within epsilon */
#define ASSERT_FLOAT_EQ(expected, actual, epsilon) do { \
test_assertions++; \
double _exp = (double)(expected); \
double _act = (double)(actual); \
double _eps = (double)(epsilon); \
if (fabs(_exp - _act) > _eps) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected: %g, Actual: %g (epsilon: %g)\n", _exp, _act, _eps); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert pointer is not NULL */
#define ASSERT_NOT_NULL(ptr) do { \
test_assertions++; \
if ((ptr) == NULL) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected non-NULL pointer\n"); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert pointer is NULL */
#define ASSERT_NULL(ptr) do { \
test_assertions++; \
if ((ptr) != NULL) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected NULL pointer\n"); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert a < b */
#define ASSERT_LT(a, b) do { \
test_assertions++; \
long long _a = (long long)(a); \
long long _b = (long long)(b); \
if (!(_a < _b)) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected %lld < %lld\n", _a, _b); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert a <= b */
#define ASSERT_LE(a, b) do { \
test_assertions++; \
long long _a = (long long)(a); \
long long _b = (long long)(b); \
if (!(_a <= _b)) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected %lld <= %lld\n", _a, _b); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert a > b */
#define ASSERT_GT(a, b) do { \
test_assertions++; \
long long _a = (long long)(a); \
long long _b = (long long)(b); \
if (!(_a > _b)) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected %lld > %lld\n", _a, _b); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Assert a >= b */
#define ASSERT_GE(a, b) do { \
test_assertions++; \
long long _a = (long long)(a); \
long long _b = (long long)(b); \
if (!(_a >= _b)) { \
printf(" [FAIL] %s\n", current_test); \
printf(" Expected %lld >= %lld\n", _a, _b); \
printf(" at %s:%d\n", __FILE__, __LINE__); \
test_failures++; \
return; \
} \
} while (0)

/* Test suite name header */
#define TEST_SUITE(name) printf("\n=== %s ===\n", name)

#endif /* TEST_FRAMEWORK_H */
80 changes: 80 additions & 0 deletions tests/test_main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* DWGSIM Unit Test Runner
*
* This file runs all unit tests for DWGSIM.
* Individual test files are included below.
*/

#include "test_framework.h"

/* Include test files here as they are added */
/* #include "test_position.c" */
/* #include "test_memory.c" */
/* #include "test_parsing.c" */
Comment on lines +10 to +13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reconsider the pattern of including .c files.

The commented examples suggest including .c files directly:

/* #include "test_position.c" */

Issues with this approach:

  • Violates standard C compilation model (separate compilation units)
  • Can cause symbol conflicts and duplicate definitions
  • Makes build dependencies unclear
  • While this pattern sidesteps the static global issue in test_framework.h (by keeping everything in one translation unit), it introduces other problems

Better alternatives:

  1. Compile test files separately and link them, fixing the static globals issue properly
  2. Use header files (.h) for test declarations if tests must be included

Consider documenting the intended test file organization pattern in the header comment or README.

🤖 Prompt for AI Agents
In tests/test_main.c around lines 10 to 13, avoid including .c test files
directly (e.g., /* #include "test_position.c" */) because that forces single
translation-unit builds and causes duplicate symbols and unclear deps; instead,
compile each test_*.c separately and link them into the test binary (or move
shared declarations into headers and include .h only), fix the static/global
symbol management in test_framework.h so separate compilation works, and add a
brief header comment or README note explaining the intended build pattern and
how to add new tests.


/*
* Placeholder tests to verify the test framework works
*/
TEST(test_framework_assert) {
ASSERT(1 == 1);
ASSERT(0 == 0);
}

TEST(test_framework_assert_eq) {
ASSERT_EQ(42, 42);
ASSERT_EQ(-1, -1);
ASSERT_EQ(0, 0);
}

TEST(test_framework_assert_ne) {
ASSERT_NE(1, 2);
ASSERT_NE(-1, 1);
}

TEST(test_framework_assert_str_eq) {
ASSERT_STR_EQ("hello", "hello");
ASSERT_STR_EQ("", "");
}

TEST(test_framework_assert_float_eq) {
ASSERT_FLOAT_EQ(3.14, 3.14, 0.001);
ASSERT_FLOAT_EQ(0.1 + 0.2, 0.3, 0.0001);
}

TEST(test_framework_assert_comparisons) {
ASSERT_LT(1, 2);
ASSERT_LE(1, 1);
ASSERT_LE(1, 2);
ASSERT_GT(2, 1);
ASSERT_GE(2, 2);
ASSERT_GE(3, 2);
}

TEST(test_framework_assert_null) {
int *ptr = NULL;
int val = 42;
ASSERT_NULL(ptr);
ASSERT_NOT_NULL(&val);
}

int main(void) {
printf("DWGSIM Unit Tests\n");
printf("========================================\n");

TEST_SUITE("Test Framework Verification");
RUN_TEST(test_framework_assert);
RUN_TEST(test_framework_assert_eq);
RUN_TEST(test_framework_assert_ne);
RUN_TEST(test_framework_assert_str_eq);
RUN_TEST(test_framework_assert_float_eq);
RUN_TEST(test_framework_assert_comparisons);
RUN_TEST(test_framework_assert_null);

/* Add test suites here as they are created */
/* TEST_SUITE("Position Calculation Tests"); */
/* Include tests from test_position.c */

TEST_SUMMARY();

return test_failures > 0 ? 1 : 0;
}