Skip to content

Tests, Clearing, Fixes#207

Merged
MatthieuDartiailh merged 15 commits intonucleic:mainfrom
GGsrvg:main
Feb 3, 2026
Merged

Tests, Clearing, Fixes#207
MatthieuDartiailh merged 15 commits intonucleic:mainfrom
GGsrvg:main

Conversation

@GGsrvg
Copy link
Contributor

@GGsrvg GGsrvg commented Jan 24, 2026

Added C++ tests:

  • One simplex test
  • All tests from "py/tests"

Removed unused includes and commented code
Added compilation options for code verification

@codecov
Copy link

codecov bot commented Jan 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.29%. Comparing base (b89f9d7) to head (4080df1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
+ Coverage   82.18%   82.29%   +0.10%     
==========================================
  Files          24       24              
  Lines        1774     1785      +11     
  Branches      242      242              
==========================================
+ Hits         1458     1469      +11     
  Misses        172      172              
  Partials      144      144              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GGsrvg
Copy link
Contributor Author

GGsrvg commented Jan 24, 2026

@MatthieuDartiailh hey, its me again
Can you review this mr?

@GGsrvg
Copy link
Contributor Author

GGsrvg commented Jan 24, 2026

Next issues could be closed
#135 fixed
#132 CMake helps with it
#60 Tests is a best documentation

@MatthieuDartiailh
Copy link
Member

Could you add a CI job to run the added C++ tests ?

@GGsrvg
Copy link
Contributor Author

GGsrvg commented Jan 24, 2026

@MatthieuDartiailh yes, I will do it. Call tests on every commit to the main branch? How do you want it to work?

@MatthieuDartiailh
Copy link
Member

Yes. You can follow the trigger used for Python jobs. However running on Linux should be enough for the C++ tests.

@GGsrvg
Copy link
Contributor Author

GGsrvg commented Jan 24, 2026

@MatthieuDartiailh I did it.

PS: Could you please take a look at why it fails with the "Generating documentation" error every time? I don't know enough Python to try to fix this.

@MatthieuDartiailh
Copy link
Member

The documentation issue comes from sphinx-tabs which is incompatible with latest sphinx version. PR are open to fix the issue but have not been merged yet.

@GGsrvg
Copy link
Contributor Author

GGsrvg commented Jan 26, 2026

What else is needed to accept this mr?

@GGsrvg
Copy link
Contributor Author

GGsrvg commented Feb 1, 2026

@MatthieuDartiailh can you suggest anything?

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

One minor point but otherwise LGTM. Sorry for the slow review.

@@ -1,13 +1,12 @@
/*-----------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

The commented code is there to easily check the relevance of using AssocVec vs a standard map.
I can understand the urge to remove commented out code but in this occurrence I would feel better to keep it. You may add a comment explaining why it is there.

@MatthieuDartiailh MatthieuDartiailh merged commit d28b726 into nucleic:main Feb 3, 2026
38 of 39 checks passed
@MatthieuDartiailh
Copy link
Member

Thanks !

@GGsrvg
Copy link
Contributor Author

GGsrvg commented Feb 4, 2026

@MatthieuDartiailh what you think about it? #207 (comment)

Can you release a new version? This is necessary so that we can use 'FetchContent' with the tag.

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.

2 participants