Skip to content

Added a comprehensive test suite for KeplerianElements - #41

Merged
RazerM merged 2 commits into
RazerM:masterfrom
mgiuca:tests
Sep 3, 2025
Merged

Added a comprehensive test suite for KeplerianElements#41
RazerM merged 2 commits into
RazerM:masterfrom
mgiuca:tests

Conversation

@mgiuca

@mgiuca mgiuca commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

Hi,

I have put together a comprehensive test suite for KeplerianElements, in order to find edge-case bugs and prove their fixes. I am making extensive use of the Orbital library in a game project, so it is of interest to me that it is working properly for all possible inputs. As such, I have been filing many bug reports over the past few days. I have fixes ready for all of these bugs, but the first order of business is to ensure a healthy test suite for these fixes.

I understand this PR is enormous, but I did want to exercise a lot of edge cases across the library (noting that I have not focused on the maneuver module), and did not see much point in sending a bunch of smaller PRs. Please review at your leisure.

Suggested commit message:

Added a comprehensive test suite for KeplerianElements.
    
Adds at least one test case for all methods and property getters and setters.
Tests specifically address edge cases and cases that are handled differently by
the underlying code (e.g. inclined vs non-inclined, circular vs non-circular
orbits).
    
Also introduces a new low-level case in test_utilities for the complicated
function eccentric_anomaly_from_mean.
    
Many of these cases are either expecting failure or commented out due to known
bugs, which have been filed on GitHub and linked from the appropriate tests. The
commented-out assertions demonstrate the correct expected behaviour once those
bugs are fixed.

The bugs that some of these tests are specifically calling out include: #18, #37, #38, #39 and #40. (Note: I intend to send one or more PRs to fix all of these in the future.)

@mgiuca
mgiuca force-pushed the tests branch 2 times, most recently from 7592260 to d37db21 Compare July 22, 2025 03:13
@mgiuca

mgiuca commented Jul 22, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for merging the other PRs. I have rebased this branch.

I should mention that I am running this locally on my Linux machine using the instructions I gave in #35 (since I could not get the tests to run using the existing instruction of python setup.py test).

Here is the output:

$ venv/bin/pytest
=========================== test session starts ===========================
platform linux -- Python 3.12.3, pytest-8.3.5, pluggy-1.5.0
rootdir: orbital
plugins: anyio-4.9.0
collected 43 items                                                        

orbital/tests/test_maneuver.py ........                             [ 18%]
orbital/tests/test_orbital.py .................................     [ 95%]
orbital/tests/test_utilities.py ..                                  [100%]

=========================== 43 passed in 0.95s ============================

Please let me know if there's anything I can do to help the review process, e.g. if you would prefer to send the new tests in smaller PRs (e.g. one for the constructors, one for the setters, one for from_state_vector, etc).

@RazerM

RazerM commented Jul 24, 2025

Copy link
Copy Markdown
Owner

I'm probably just going to give you merge rights. After I merged a couple of your PRs I started bringing the general project setup up to date so that it's easy for you to run tests etc. (#35 is not the right approach)

I'll try get it finished and pushed.

@mgiuca

mgiuca commented Jul 29, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for looking into this. I understand this is an old project and might not be something you want to have to update, so it is appreciated.

I will hold off until you do that setup, then re-set-up on my end and rebase this.

After that, I have some fixes to apply, particularly for from_state_vector issues.

@RazerM

RazerM commented Jul 29, 2025

Copy link
Copy Markdown
Owner

#42 was merged, see https://orbitalpy.readthedocs.io/en/latest/installation.html#development-environment for how to run tests etc.

@mgiuca

mgiuca commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

Got it. Thanks for doing what looks like it must have been a huge job to modernize the Python setup process and documentation.

It will take me awhile to merge my changes on top so please bear with me. Can I just confirm that you reformatted all the code with:

ruff format . (with no additional parameters)?

So that I can use the same formatting and simplify the merge process.

Thanks.

@RazerM

RazerM commented Aug 7, 2025

Copy link
Copy Markdown
Owner

You can do pre-commit run --all-files. I also suggest pre-commit install to make it run when you commit.

Use uv tool install pre-commit if you don’t have it

But yes ruff format is one part of the lint/formatting configuration

Adds at least one test case for all methods and property getters and setters.
Tests specifically address edge cases and cases that are handled differently by
the underlying code (e.g. inclined vs non-inclined, circular vs non-circular
orbits).

Also introduces a new low-level case in test_utilities for the complicated
function eccentric_anomaly_from_mean.

Many of these cases are either expecting failure or commented out due to known
bugs, which have been filed on GitHub and linked from the appropriate tests. The
commented-out assertions demonstrate the correct expected behaviour once those
bugs are fixed.
@mgiuca

mgiuca commented Aug 12, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for giving those instructions, it was very helpful. I'm really impressed with the new modern build infra you've set up.

I have rebased (just squashed all commits down) onto the latest version and updated my code to pass all the presubmit checks. I think this should be good to commit now, and then I can start sending out all the fixes I have in the pipeline. Thanks for your patience!

@mgiuca

mgiuca commented Sep 1, 2025

Copy link
Copy Markdown
Contributor Author

Hi @RazerM, just a friendly ping here to see if you are waiting on me for any changes or updates on this? I think I have done all I can do without your input (it says merging is blocked without approval). Cheers.

@RazerM
RazerM merged commit 5d7b1de into RazerM:master Sep 3, 2025
17 checks passed
@mgiuca
mgiuca deleted the tests branch September 16, 2025 06:16
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