Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Use fixtures in Tests - test_twobody and test_angles #504

Merged
merged 4 commits into from Dec 22, 2018

Conversation

shreyasbapat
Copy link
Member

@shreyasbapat shreyasbapat commented Dec 6, 2018

Let's do this step by step. I will make several different pull requests which will eventually enable usage of fixtures in every place where it is better for us to use.

@astrojuanlu
Copy link
Member

If you want some early suggestions:

  • Tell me about your plans, so we can discuss :)
  • Start small. In the original fixtures issue it's said that "There are a lot of repeated code in the tests, mainly sample data for orbits" Consider using fixtures in tests #447, so perhaps in test_angles, test_twobody or test_propagation there are also opportunities.
  • Leave the plotting module for a separate PR, as I have some unfinished work that might affect all of it...

@shreyasbapat
Copy link
Member Author

Sure. I will work on the other tests. I was just trying out. I will discuss with you today. :)

@codecov
Copy link

codecov bot commented Dec 7, 2018

Codecov Report

Merging #504 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #504   +/-   ##
=======================================
  Coverage   86.04%   86.04%           
=======================================
  Files          49       49           
  Lines        2185     2185           
  Branches      165      165           
=======================================
  Hits         1880     1880           
  Misses        265      265           
  Partials       40       40

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cc569e...614ddd6. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 7, 2018

Codecov Report

Merging #504 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #504   +/-   ##
=======================================
  Coverage   86.07%   86.07%           
=======================================
  Files          49       49           
  Lines        2191     2191           
  Branches      166      166           
=======================================
  Hits         1886     1886           
  Misses        265      265           
  Partials       40       40

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72eff1b...c0ce169. Read the comment docs.

@shreyasbapat shreyasbapat changed the title [WIP] Use fixtures in Tests Use fixtures in Tests - test_twobody and test_angles Dec 7, 2018
@shreyasbapat
Copy link
Member Author

@Juanlu001 Please review whenever you get a chance. If you feel something is less than what it should be, please let me know, I will fix it. :)

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks @shreyasbapat for this PR and sorry for the huge delay 🙏 I left a couple of comments. Also, this will need a rebase.

from poliastro.core.elements import coe2rv, coe2mee, rv2coe
from poliastro.twobody.equinoctial import mee2coe


def test_convert_between_coe_and_rv_is_transitive():
k = Earth.k.to(u.km**3 / u.s**2).value # u.km**3 / u.s**2
@pytest.fixture(scope="module")
Copy link
Member

Choose a reason for hiding this comment

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

Didn't know about scope="module", nice!

https://docs.pytest.org/en/3.8.0/fixture.html#scope-sharing-a-fixture-instance-across-tests-in-a-class-module-or-session

However, I believe that they are used for expensive fixtures, so I'd rather remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! will do so!

@@ -7,6 +7,24 @@
from poliastro.twobody import angles


@pytest.fixture(scope="module")
Copy link
Member

Choose a reason for hiding this comment

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

In this particular case, let's use ANGLES_DATA just as a module variable, without using a fixture.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay!

@shreyasbapat
Copy link
Member Author

@Juanlu001 I have rebased as well as made the suggested changes :)

@astrojuanlu astrojuanlu merged commit 9e8dd40 into poliastro:master Dec 22, 2018
@ghost ghost removed the 2 - In Progress label Dec 22, 2018
@astrojuanlu
Copy link
Member

Merged! Thanks for your patience :)

@shreyasbapat shreyasbapat deleted the tests branch December 25, 2018 22:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants