Skip to content

Fix bugs in core classes + add test suite and CI pipeline#9

Draft
kartikeyg0104 wants to merge 1 commit intoEAPD-DRB:mainfrom
kartikeyg0104:feature/code-quality-bugfix-testing
Draft

Fix bugs in core classes + add test suite and CI pipeline#9
kartikeyg0104 wants to merge 1 commit intoEAPD-DRB:mainfrom
kartikeyg0104:feature/code-quality-bugfix-testing

Conversation

@kartikeyg0104
Copy link
Contributor

@kartikeyg0104 kartikeyg0104 commented Feb 22, 2026

Closes #10

Bug fixes

  1. EmissionActivityRatio typo in Config.py — Fixed 'e''t' (Python string concatenation → 'et') to 'e','t' in PARAMETERS_C. This was silently corrupting parameter validation dimensions.

  2. File handle leaks in FileClass.py — Refactored all four static methods (readFile, writeFile, writeFileUJson, readParamFile) from manual open()/close() to with open(...) as f: context managers. Previous code leaked file handles when exceptions occurred between open() and close().

  3. CustomThread.join() missing timeout — Added timeout=None parameter to match Thread.join() signature. Previously, thread.join(timeout=5) would raise TypeError.

Testing infrastructure

  1. Added pytest test suite — 25 tests across 3 test files:

    • tests/test_config.py (9 tests) — validates PARAMETERS_C dimensions, config paths, group tuples, and mapping key consistency
    • tests/test_file_class.py (10 tests) — JSON read/write round-trips, missing file handling, invalid JSON handling, overwrite behavior
    • tests/test_custom_thread.py (6 tests) — return values, args/kwargs passing, timeout acceptance, timeout behavior
  2. Added GitHub Actions CI (.github/workflows/ci.yml):

    • Lint job: flake8 checking for syntax errors (E9, F63, F7, F82)
    • Test job: pytest across matrix of Python 3.10/3.11/3.12 × Ubuntu/macOS/Windows
  3. Added pytest.ini — configures test discovery paths and naming conventions

  4. Added __pycache__/ to .gitignore

Validation

$ pytest tests/ -v --tb=short
============================== 25 passed in 0.73s ==============================

All 25 tests pass locally on macOS with Python 3.12.

…mThread.join timeout, add pytest suite + CI

- Fix 'e''t' string concatenation bug in PARAMETERS_C['EmissionActivityRatio']
  that silently produced ['r','et','y','m'] instead of ['r','e','t','y','m']
- Refactor FileClass to use context managers (with statement) for all file I/O
  to prevent file handle leaks on exceptions
- Fix CustomThread.join() to accept optional timeout parameter matching
  Thread.join() signature
- Add pytest test suite with 25 tests covering FileClass, Config, and
  CustomThread
- Add GitHub Actions CI workflow running lint + tests on Python 3.10-3.12
  across Ubuntu, macOS, and Windows
- Add pytest.ini configuration
- Add __pycache__/ to .gitignore
@SeaCelo
Copy link
Collaborator

SeaCelo commented Feb 22, 2026

Thank you. Please keep this as a draft fo now until we examine the issue and weather it should be upstream or here.

@SeaCelo SeaCelo marked this pull request as draft February 22, 2026 23:37
@SeaCelo
Copy link
Collaborator

SeaCelo commented Feb 22, 2026

I like the testing infrastructure. Can you open that as a separate issue?

@kartikeyg0104
Copy link
Contributor Author

I like the testing infrastructure. Can you open that as a separate issue?

Hi @SeaCelo, thanks for the feedback! I've opened #14 to track the testing infrastructure separately as you suggested. Happy to keep this PR as a draft, I'll wait for your decision on whether the bug fixes should go upstream or stay here before making any further changes. Let me know if there's anything else you'd like adjusted.

@NamanmeetSingh
Copy link

Hey, great catch on the EmissionActivityRatio dimension typo! Since we are laying the groundwork for the OG-CLEWS integration, I had a quick architectural thought regarding the data pipelines when you port this:

1. Dimension Extensibility (test_config.py):

In test_all_dimension_keys_are_single_chars_or_known, the known set strictly maps to CLEWS/OSeMOSYS dimensions (r, t, f, etc.). As we integrate OG-Core, we will need to support macroeconomic dimensions (like S for age cohorts, J for income groups, I for industries). Are we planning to expand PARAMETERS_C to hold these, or will we spin up a separate PARAMETERS_OG dictionary? Either way, we might want to flag this test so it doesn't block the CI when we start mapping the coupled-model schemas.

2. Large Dataset Memory Handling (FileClass.py):

Love the move to context managers! One quick optimization for the model runs: using json.loads(f.read()) pulls the entire text into memory before parsing, which can cause RAM spikes when loading massive multi-country scenario files. Switching to json.load(f) allows the parser to stream directly from the file, which is much safer for our heavy data payloads.

Really clean refactor overall! The file handle leak fix is definitely going to save us some debugging headaches during parallel runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Fix bugs in core classes + add test suite and CI pipeline

3 participants