Skip to content

Rework coverage for NMODL ODE solver and pybind - #3544

Merged
nrnhines merged 8 commits into
masterfrom
jelic/refactor_coverage
Jan 22, 2026
Merged

Rework coverage for NMODL ODE solver and pybind#3544
nrnhines merged 8 commits into
masterfrom
jelic/refactor_coverage

Conversation

@JCGoran

@JCGoran JCGoran commented Jul 25, 2025

Copy link
Copy Markdown
Collaborator

Rework coverage so each invocation of nrnivmodl -coreneuron creates a coverage report if the ODE solver is used. Note that the coverage files may be littered across the source directory; this is due to the fact that nrnivmodl does too much "fixing" of paths of modfiles, so it's basically impossible to run it from the build directory itself (it does a cd beforehand).
Also note that the coverage tool is broken (see below comment), so we do not get coverage of all of the lines.

Goran Jelic-Cizmek added 3 commits July 25, 2025 15:56
Put back whatever hackery it used before to collect the mod files, if
some of the files are missing coverage I really don't care.
@github-actions

Copy link
Copy Markdown
Contributor

✔️ e0e83ea -> artifacts URL

@codecov

codecov Bot commented Jul 26, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.31%. Comparing base (9d29cc9) to head (ca5369c).
⚠️ Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3544      +/-   ##
==========================================
+ Coverage   68.09%   68.31%   +0.21%     
==========================================
  Files         689      689              
  Lines      111038   111038              
==========================================
+ Hits        75615    75851     +236     
+ Misses      35423    35187     -236     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@azure-pipelines

Copy link
Copy Markdown

✔️ e0e83ea -> Azure artifacts URL

@JCGoran

JCGoran commented Jul 28, 2025

Copy link
Copy Markdown
Collaborator Author

Okay, I am very confused how codecov is collecting and computing the coverage. For instance, I don't think this makes any sense:

Screenshot 2025-07-28 at 10 13 25

This is taken from
https://app.codecov.io/gh/neuronsimulator/nrn/commit/e0e83ea869bea4aaebeb0ea77f5daa7103e6c6bc/blob/share/lib/python/neuron/nmodl/ode.py?dropdown=coverage#L132.

Note that one line (132) has no coverage, and then the line immediately following it (133), at the same indentation level, does have coverage? I have no idea what's going on here...

@github-actions

Copy link
Copy Markdown
Contributor

✔️ c81be96 -> artifacts URL

also add comment about using single triple quotes
Comment thread src/nmodl/pybind/CMakeLists.txt
@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2025

Copy link
Copy Markdown

@JCGoran
JCGoran marked this pull request as ready for review August 4, 2025 10:18
@JCGoran
JCGoran requested a review from nrnhines August 4, 2025 10:18
@github-actions

github-actions Bot commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

✔️ 5cf9a74 -> artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ 47fa629 -> Azure artifacts URL

@mgeplf

mgeplf commented Sep 3, 2025

Copy link
Copy Markdown
Collaborator

Note that one line (132) has no coverage, and then the line immediately following it (133), at the same indentation level, does have coverage?

I had a quick look; examining the .coverage file seems to show more proper coverage (at least 133 and 132 are covered):
coverage report --data-file=/home/mgevaert/src/nrn/build/test/nmodl/transpiler/unit/ode/.coverage -m gives

Name                                                    Stmts   Miss  Cover   Missing
-------------------------------------------------------------------------------------
....
lib/python/neuron/nmodl/ode.py                            244    118    52%   26, 34, 62-65, 69-72, 76-82, 86-91, 95-101, 105-110, 114-120, 137-140, 200-218, 245-252, 296-354, 383, 412-447, 526-535, 577-584

I wonder if it makes more sense to have coverage just make the .coverage file, and do a post process step to create the single .xml, that way other invocations wouldn't impact it?.

Comment thread src/nmodl/pybind/CMakeLists.txt
Comment thread test/nmodl/transpiler/unit/CMakeLists.txt
@sonarqubecloud

Copy link
Copy Markdown

@azure-pipelines

Copy link
Copy Markdown

✔️ ca5369c -> Azure artifacts URL

@github-actions

Copy link
Copy Markdown
Contributor

✔️ ca5369c -> artifacts URL

@nrnhines
nrnhines merged commit f2d4ed3 into master Jan 22, 2026
65 of 69 checks passed
@nrnhines
nrnhines deleted the jelic/refactor_coverage branch January 22, 2026 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants