Skip to content

Move PositiveNumbers tests to new suite - #901

Draft
sdraeger wants to merge 1 commit into
JuliaManifolds:masterfrom
sdraeger:transfer-positive-numbers-tests
Draft

Move PositiveNumbers tests to new suite#901
sdraeger wants to merge 1 commit into
JuliaManifolds:masterfrom
sdraeger:transfer-positive-numbers-tests

Conversation

@sdraeger

Copy link
Copy Markdown

Summary

  • Move the PositiveNumbers tests from test/manifolds-old to test/manifolds.
  • Replace the old Manifolds.test_manifold calls with Manifolds.Test.test_manifold coverage for PositiveNumbers() and PositiveVectors(2).
  • Keep the explicit constructor, representation, metric conversion, Hessian, volume, random, and field-parameter checks from the old file.

Part of #870.

Validation

  • julia --project=test test/manifolds/test_positive_numbers.jl
  • MANIFOLDS_TEST_SET=manifolds julia --project=test test/runtests.jl was attempted twice and timed out after 20 minutes in unrelated old symplectic tests; the run reached and passed manifolds/test_positive_numbers.jl before timing out.

@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.95%. Comparing base (5bb7aa2) to head (e14ef2b).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##            master     #901      +/-   ##
===========================================
- Coverage   100.00%   99.95%   -0.05%     
===========================================
  Files           98       98              
  Lines         9792     9792              
===========================================
- Hits          9792     9788       -4     
- Misses           0        4       +4     

☔ View full report in Codecov by Harness.
📢 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.

@kellertuer

kellertuer commented May 28, 2026

Copy link
Copy Markdown
Member

Thats a nice start. Thanks!

Several of the functions in the first block are also available in the new test suite and do no longer have to be tested separately.
The validation sounds a bit AI-ish (ran for 20 minutes? The whole Manifolds.jl tests on my machine runs 25 or so minutes) How much AI was involved here? Just curious and like transparency, especially on AI things.
Besides that let's wait for the test results & coverage.

Your last name sounds German, I am originally from Dräger-Town (or Marzipanien or just good old Lübeck)!

@sdraeger

Copy link
Copy Markdown
Author

Thank you! Yes, this was AI-assisted, and I reviewed the resulting diff and ran the targeted PositiveNumbers test locally. I agree the validation wording was awkward and will change it accordingly.

I’ll also trim the first block to keep only the constructor/extra behavior checks that are not already covered by Manifolds.Test.test_manifold.

@kellertuer

Copy link
Copy Markdown
Member

Thanks for the transparency.
I was mainly curious, because by now I often spent time fixing other peoples AI slop (both in my daily job and here in open source software. That does not mean, your code is bad, as I wrote it is a very good start.

Let me know when you have rechecked it, I can help looking through the rest as well. But for example repr can be tested in the suite, see https://juliamanifolds.github.io/Manifolds.jl/stable/misc/testsuite/#Manifolds.Test.test_repr, so line 7 can be moved; provide the expectation (the second dict) and it also checks that the string is right, cf

repr => "Sphere(2)",

The reason to really only check edge cases outside that function is, that in a next step, “filtering” tests gets easier – one idea is to have a short integration test for example.

Also for the functions, we are using runic for code formatting, so feel free to group functions reasonably together on lines.
Good that you kept the structure with the using line upfront, that way a single manifold test can be run individually.

Thanks for working on this, I know it is a bit dull (and hence maybe even suited for a bit of AI), but a good thing to slowly revise for a bit faster and flexible CI.

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