Skip to content

Conversation

@aje-valeo
Copy link
Collaborator

Add tests for INI parsing.

  • Python tests
  • C test (inih library)
    • Using the local library (CMake assumes library exists)

@aje-valeo aje-valeo self-assigned this Nov 4, 2025
@aje-valeo aje-valeo added the bug Something isn't working label Nov 4, 2025
@aje-valeo aje-valeo requested review from julianh65, kevin-joseph-ai and l1onh3art88 and removed request for eugenevinitsky and julianh65 November 4, 2025 14:42
@aje-valeo aje-valeo force-pushed the ini_tester branch 2 times, most recently from 3fb0d16 to 6922ca0 Compare November 6, 2025 08:46
- Check values from default.ini
- Check values from drive.ini
- Additional checks for comments capabilities
- Add CMake project to configure, build and test
- Test value parsing
- Test comments format
- Add comments for (un)expected results
- Ini parsing tests
- Update comments to clarify intent
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive test coverage for INI file parsing in both Python and C. The tests verify that the configuration loading functionality correctly parses INI files, handles comments, and supports command-line overrides.

Key changes:

  • Added Python unit tests for INI configuration loading with various test scenarios
  • Added C-based tests using the inih library to verify INI parsing behavior
  • Created GitHub Actions workflow to run both Python and C tests

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_drive_config.py Python unit tests for configuration loading, comment handling, and CLI overrides
tests/ini_parser/test_drive.ini Test INI file with various configurations and comment styles
tests/ini_parser/ini_tester.c C test implementation using inih library to verify parsing behavior
tests/ini_parser/build_n_test.sh Build script for compiling and running C tests
tests/ini_parser/CMakeLists.txt CMake configuration for building C test executable
.github/workflows/utest.yml GitHub Actions workflow to run both Python and C tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +37 to +59
} if (MATCH("env", "action_type")) {
env_config->action_type = strdup(value);
} if (MATCH("env", "num_maps")) {
env_config->num_maps = atoi(value);
} if (MATCH("policy", "input_size")) {
env_config->input_size = atoi(value);
} if (MATCH("policy", "hidden_size")) {
env_config->hidden_size = atoi(value);
} if (MATCH("test", "key1")) {
env_config->key1 = atoi(value);
} if (MATCH("test", "key2")) {
env_config->key2 = atof(value);
} if (MATCH("test", "key3")) {
env_config->key3 = atoi(value);
} if (MATCH("test", "key31")) {
env_config->key3 = atof(value);
} if (MATCH("test", "key4")) {
env_config->key4 = strdup(value);
} if (MATCH("test", "key5")) {
env_config->key5 = strdup(value);
} if (MATCH("test", "key6")) {
env_config->key6 = atoi(value);
} if (MATCH("test", "key7")) {
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

All the if statements from lines 37-60 should be 'else if' statements. Currently, the logic continues checking all conditions even after finding a match, and the final 'else' block at line 61 only applies to the last 'if' statement (key7), not to the entire chain as intended. This means unmatched keys will incorrectly return 1 instead of 0.

Suggested change
} if (MATCH("env", "action_type")) {
env_config->action_type = strdup(value);
} if (MATCH("env", "num_maps")) {
env_config->num_maps = atoi(value);
} if (MATCH("policy", "input_size")) {
env_config->input_size = atoi(value);
} if (MATCH("policy", "hidden_size")) {
env_config->hidden_size = atoi(value);
} if (MATCH("test", "key1")) {
env_config->key1 = atoi(value);
} if (MATCH("test", "key2")) {
env_config->key2 = atof(value);
} if (MATCH("test", "key3")) {
env_config->key3 = atoi(value);
} if (MATCH("test", "key31")) {
env_config->key3 = atof(value);
} if (MATCH("test", "key4")) {
env_config->key4 = strdup(value);
} if (MATCH("test", "key5")) {
env_config->key5 = strdup(value);
} if (MATCH("test", "key6")) {
env_config->key6 = atoi(value);
} if (MATCH("test", "key7")) {
} else if (MATCH("env", "action_type")) {
env_config->action_type = strdup(value);
} else if (MATCH("env", "num_maps")) {
env_config->num_maps = atoi(value);
} else if (MATCH("policy", "input_size")) {
env_config->input_size = atoi(value);
} else if (MATCH("policy", "hidden_size")) {
env_config->hidden_size = atoi(value);
} else if (MATCH("test", "key1")) {
env_config->key1 = atoi(value);
} else if (MATCH("test", "key2")) {
env_config->key2 = atof(value);
} else if (MATCH("test", "key3")) {
env_config->key3 = atoi(value);
} else if (MATCH("test", "key31")) {
env_config->key3 = atof(value);
} else if (MATCH("test", "key4")) {
env_config->key4 = strdup(value);
} else if (MATCH("test", "key5")) {
env_config->key5 = strdup(value);
} else if (MATCH("test", "key6")) {
env_config->key6 = atoi(value);
} else if (MATCH("test", "key7")) {

Copilot uses AI. Check for mistakes.
} if (MATCH("test", "key3")) {
env_config->key3 = atoi(value);
} if (MATCH("test", "key31")) {
env_config->key3 = atof(value);
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

This should assign to 'env_config->key31' instead of 'env_config->key3'. The current code incorrectly overwrites key3 when key31 is parsed.

Suggested change
env_config->key3 = atof(value);
env_config->key31 = atof(value);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants