Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: Splitting conftest.py into several smaller files. #556

Merged
merged 8 commits into from
Feb 19, 2024

Conversation

lucasfourier
Copy link
Contributor

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)
  • ReadMe, Docs and GitHub updates
  • Other (please describe):

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

Currently, conftest.py is unique and top level within the tests directory. However, as observed by @Gui-FernandesBR in #447, the file was getting too big to be properly maintained.

New behavior

Several .py modules were created in order to split the fixtures into modules which results in easier maintenance and proper organization.

Breaking change

  • Yes
  • No

Additional information

A bit more info regarding this can be found here:
https://stackoverflow.com/questions/27064004/splitting-a-conftest-py-file-into-several-smaller-conftest-like-parts

@lucasfourier lucasfourier added the Tests Regarding Tests label Feb 13, 2024
@lucasfourier lucasfourier requested a review from a team as a code owner February 13, 2024 21:52
@Gui-FernandesBR Gui-FernandesBR linked an issue Feb 13, 2024 that may be closed by this pull request
@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to develop February 13, 2024 21:57
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Feb 13, 2024
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9da1542) 72.47% compared to head (79aa3b6) 72.47%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #556   +/-   ##
========================================
  Coverage    72.47%   72.47%           
========================================
  Files           59       59           
  Lines         9567     9567           
========================================
  Hits          6934     6934           
  Misses        2633     2633           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Gui-FernandesBR Gui-FernandesBR changed the title Splitting conftest.py into several smaller files. TST: Splitting conftest.py into several smaller files. Feb 13, 2024
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Good work, the new structure is just way better than the current one. more robust and scalable.

Please discuss on the comments I've added and let me know if there's something not so clear.

Can you apply black before commiting? Out Makefile has been updated recently if you wanna use the make black command.

Also, can you confirm the slow tests are passing locally?

Once again, good work here!

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/fixtures/hybrid/__init__.py Outdated Show resolved Hide resolved
tests/fixtures/motor/motor_fixtures.py Outdated Show resolved Hide resolved
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Thank you again for your effort, I really appreciate it, @lucasfourier .

I've added a few more comments on what I think is not ideal yet, could you check it out please?

tests/fixtures/motor/motor_fixtures.py Outdated Show resolved Hide resolved
tests/fixtures/hybrid/hybrid_fixtures.py Show resolved Hide resolved
tests/fixtures/motor/liquid_fixtures.py Outdated Show resolved Hide resolved
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Great PR, thank you so much for your contribution, @lucasfourier !!

Tests are now more organized and faster.

You can merge this PR once the tests are all green in github actions. Also, please feel free to close the issue associated with this PR.

@lucasfourier lucasfourier merged commit e7b17de into develop Feb 19, 2024
11 checks passed
@lucasfourier lucasfourier deleted the mnt/refactoring-fixtures branch February 19, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tests Regarding Tests
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

TST: separate tests/conftest.py into smaller files
3 participants