Increase ARC's code coverage statistics. by kfir4444 · Pull Request #835 · ReactionMechanismGenerator/ARC

kfir4444 · 2026-03-16T12:36:00Z

This PR related to the low (~67%) code coverage with the tests, since the inclusion of the molecule branch.
The lowered coverage is caused by the molecule module being unreachable to codecov due to the compilation. This is evident in codecov, where it seems that tests passes 100% but the modules related to the tests appear at 0.0%. This is solved by the using the CYTHON_TRACE macro. Though, due to it lowereing performence by ~10x, it is enabled only during CI via costum environment flag.
In addition, tests were added to the statmech module (specifically ARKANE), species, utils and job modules.

- Enabled allow_charged_fragments in xyz2mol when a non-zero charge is specified. - Fixed a TypeError that occurred when xyz2mol returned None for certain charged species. - Added a comprehensive test suite in arc/species/xyz_to_smiles_test.py covering: - Simple molecules (Water, Methane, Ethylene) - Charged species (OH-) - Triple bonds (Acetylene) - Chirality (S-1-fluoro-1-chloroethane) - Huckel vs non-Huckel perception

arc/job/factory_test.py

arc/statmech/arkane_test.py

Copilot

Pull request overview

This PR aims to improve ARC’s reported code coverage by making Cython-compiled modules traceable in CI (so codecov can attribute executed lines correctly) and by adding additional unit tests across several modules.

Changes:

Enable Cython line-tracing during CI builds via an ARC_COVERAGE flag and configure coverage.py to use the Cython coverage plugin.
Add new unit tests for utilities (delete), species (xyz_to_smiles), and job/parser factories.
Expand Arkane/statmech unit tests (including a mocked thermo computation path).

Reviewed changes

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

Show a summary per file

File	Description
`setup.py`	Adds an `ARC_COVERAGE`-gated Cython `linetrace` + `CYTHON_TRACE` macro configuration for coverage builds.
`arc/utils/delete_test.py`	Introduces unit tests for CLI arg parsing and basic `main()` behaviors in `arc.utils.delete`.
`arc/statmech/arkane_test.py`	Adds factory + mocked thermo tests; adjusts directory creation behavior.
`arc/species/xyz_to_smiles_test.py`	Adds unit tests covering several xyz→SMILES cases including charge and chirality.
`arc/species/xyz_to_smiles.py`	Allows charged fragments when `charge != 0` to support charged-species perception.
`arc/job/factory_test.py`	Adds tests for job and ESS factory error paths and successful instantiation via patched registries.
`.github/workflows/ci.yml`	Sets `ARC_COVERAGE=1` during compile and test steps to produce traced Cython builds in CI.
`.coveragerc`	Enables `Cython.Coverage` plugin so coverage can map executed Cython lines back to sources.

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

arc/statmech/arkane_test.py

arc/job/factory_test.py

alongd · 2026-03-16T17:21:11Z

@kfir4444, thanks!
Can you add a codecov indicator to all PRs, so we can see the diff in cov?

alongd · 2026-03-16T17:24:50Z

Can you explain the lowering performance by ~10x part?

alongd

Thanks! I left some comments.
I think we should avoid mock tests whenever possible, and use our real object, unless there's an operational reason to do otherwise

alongd · 2026-03-16T17:27:30Z

arc/utils/delete_test.py

+from unittest.mock import patch, MagicMock
+from arc.utils.delete import parse_command_line_arguments, main
+from arc.exceptions import InputError
+


please add another line break before the class

alongd · 2026-03-16T17:28:49Z

arc/utils/delete_test.py

+        mock_isfile.return_value = False
+        main()
+        mock_local.assert_called_with(jobs=None)
+


please add another line break

alongd · 2026-03-16T17:31:33Z

arc/utils/delete_test.py

+    @patch('arc.utils.delete.delete_all_local_arc_jobs')
+    @patch('arc.utils.delete.parse_command_line_arguments')
+    @patch('arc.utils.delete.os.path.isfile')
+    def test_main_all(self, mock_isfile, mock_parse, mock_local, mock_remote):


Can you explain the role of mock_isfile, mock_parse, mock_local, mock_remote?

Mocks are used to isolate the main deletion logic from the actual system environment,
preventing unintended file deletion or execution of command-line parsing during tests.

mock_parse: Simulates command-line arguments (sys.argv) parsing.

mock_local: Simulates the deletion of local ARC jobs.

mock_remote: Simulates the deletion of remote ARC jobs via SSH.

mock_isfile: Simulates the presence or absence of the initiated_jobs.csv database file.

Also added as a docs string to the test

alongd · 2026-03-16T17:34:17Z

arc/statmech/arkane_test.py

+    @patch('arc.statmech.arkane.create_statmech_dir')
+    @patch('arc.statmech.arkane.execute_command')
+    @patch('arc.statmech.arkane.run_arkane')
+    def test_compute_thermo_mocked(self, mock_run_arkane, mock_execute_command, mock_create_statmech_dir):


I don't like mock tests in general. Why not run real data? shouldn't take long to execute

alongd · 2026-03-16T17:35:13Z

arc/statmech/arkane_test.py

        for folder in ['arkane_tests_delete', 'arkane_input_tests_delete']:
            shutil.rmtree(os.path.join(ARC_TESTING_PATH, folder), ignore_errors=True)

-


we have 2 breaks before the if __name__ == '__main__':

alongd · 2026-03-16T17:36:11Z

arc/job/factory_test.py

+from arc.parser.adapter import ESSAdapter, ESSEnum
+from arc.species import ARCSpecies
+
+class MockJobAdapter(JobAdapter):


I prefer not to use mock tests, where possible. Let's test our real objects

kfir4444 · 2026-03-18T05:34:52Z

Can you explain the lowering performance by ~10x part?

CYTHON_TRACE is a compile-time macro that enables line-level tracing hooks inside generated C code from Cython. Those hooks are what allow coverage tools (like Codecov via coverage.py) to “see” execution inside .pyx files. Tracing inserts a call per line → significant slowdown. This is why I only enable it under CI workflow.

codecov · 2026-03-18T07:44:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.60%. Comparing base (49931e8) to head (7c117e7).
⚠️ Report is 12 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (49931e8) and HEAD (7c117e7). Click for more details.

HEAD has 4 uploads less than BASE

Flag BASE (49931e8) HEAD (7c117e7)

unittests 3 1

functionaltests 3 1

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #835      +/-   ##
==========================================
- Coverage   67.38%   61.60%   -5.78%     
==========================================
  Files         154      159       +5     
  Lines       46095    46438     +343     
  Branches     8486     8505      +19     
==========================================
- Hits        31061    28610    -2451     
- Misses      13179    15517    +2338     
- Partials     1855     2311     +456

Flag	Coverage Δ
functionaltests	`61.60% <ø> (-5.78%)`	⬇️
unittests	`61.60% <ø> (-5.78%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Created arc/utils/delete_test.py to improve coverage of arc/utils/delete.py. - Added tests for parse_command_line_arguments() covering various flags (-p, -j, -a). - Added mocked tests for main() to verify proper handling of missing arguments and the --all flag without side effects.

- Resolved FileExistsError and FileNotFoundError in parallel testing (xdist) by using exist_ok=True and better directory management. - Added a mocked test for compute_thermo() that creates necessary dummy files and bypasses external Arkane/bash script execution for reliability. - Added unit tests for statmech_factory() to ensure proper registration and instantiation of statmech adapters.

- Created arc/job/factory_test.py to test factory registration and instantiation logic. - Implemented MockJobAdapter and MockESSAdapter to test job_factory() and ess_factory() without external ESS software dependencies. - Added tests for error handling: unregistered labels, invalid argument types, and missing required parameters. - This provides comprehensive coverage for both arc/job/factory.py and arc/parser/factory.py.

- Tracing is now enabled only when the ARC_COVERAGE environment variable is set to 1. - This allows for accurate coverage reporting in CI while maintaining full performance in production builds.

This line is the "bridge" that allows the Python coverage tool to understand and read compiled C code.

Chenged the test's api accordingly

…y cleanup

arc/statmech/arkane_test.py

 import os
 import shutil
 import unittest
+from unittest.mock import patch


To fix an unused import, the standard approach is to remove the import statement for the unused symbol, leaving all other imports and functionality unchanged. This eliminates the unnecessary dependency and silences the static analysis warning without affecting runtime behavior.

In this specific case, in arc/statmech/arkane_test.py, the line from unittest.mock import patch is not used anywhere in the shown code. The best fix is to delete that import line entirely. No additional code, methods, or definitions are needed because we are only removing an unused symbol, not adding new behavior. Concretely, remove line 11 that imports patch, keeping all other imports and code as-is.

github-advanced-security bot found potential problems Mar 16, 2026

View reviewed changes

arc/job/factory_test.py Fixed Show fixed Hide fixed

arc/job/factory_test.py Fixed Show fixed Hide fixed

arc/job/factory_test.py Fixed Show fixed Hide fixed

arc/job/factory_test.py Fixed Show fixed Hide fixed

arc/statmech/arkane_test.py Fixed Show fixed Hide fixed

alongd requested a review from Copilot March 16, 2026 12:43

Copilot started reviewing on behalf of alongd March 16, 2026 12:43 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

arc/statmech/arkane_test.py Outdated Show resolved Hide resolved

arc/job/factory_test.py Outdated Show resolved Hide resolved

kfir4444 force-pushed the more_tests branch from c6069d6 to b93a40c Compare March 16, 2026 13:32

alongd reviewed Mar 16, 2026

View reviewed changes

github-actions bot added the Module Functional label Mar 18, 2026

kfir4444 added 8 commits March 18, 2026 10:21

Enable conditional Cython tracing in setup.py

8a8a8eb

- Tracing is now enabled only when the ARC_COVERAGE environment variable is set to 1. - This allows for accurate coverage reporting in CI while maintaining full performance in production builds.

Added Cython.Coverage to ci

561075b

This line is the "bridge" that allows the Python coverage tool to understand and read compiled C code.

added Cython.Coverage plugin

58b4dc8

updated codecov

b384e03

Chenged the test's api accordingly

Fix race condition in TestRestart by individualizing project director…

7c117e7

…y cleanup

kfir4444 force-pushed the more_tests branch from d0d9cd6 to 7c117e7 Compare March 18, 2026 10:11

github-advanced-security bot found potential problems Mar 18, 2026

View reviewed changes

		for folder in ['arkane_tests_delete', 'arkane_input_tests_delete']:
		shutil.rmtree(os.path.join(ARC_TESTING_PATH, folder), ignore_errors=True)

Conversation

kfir4444 commented Mar 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

alongd commented Mar 16, 2026

Uh oh!

alongd commented Mar 16, 2026

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

alongd Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

alongd Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

alongd Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

kfir4444 Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

alongd Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

alongd Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

alongd Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

kfir4444 commented Mar 18, 2026

Uh oh!

codecov bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Check notice

Copilot Autofix

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 18, 2026 •

edited

Loading