Skip to content

Fixed the issue with dummy variable #43

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TheRGuy9201
Copy link

@TheRGuy9201 TheRGuy9201 commented Jul 1, 2025

PR: Improve Testing Framework and Clean Repository Structure

Overview

This PR improves the WeatherRoutingTool testing framework by addressing test warnings, configuring proper test output handling, and cleaning up repository structure. These changes enhance the developer experience by reducing noise in test output and ensuring consistent test execution across environments. This addresses the issue number #41.

Changes

1. Warning Suppression

  • Added warning filters in pytest.ini and conftest.py to suppress non-critical third-party warnings
  • Targeted specific warnings from dependencies like numpy, geopandas, pkg_resources, and pyogrio
  • Maintained useful warnings while eliminating noise from test output

2. Test Output Management

  • Modified tests that generate PNG visualizations to use the project's figure path infrastructure
  • Test-generated plots now respect the WRT_FIGURE_PATH environment variable for consistent output location
  • Added proper cleanup of temporary test files

3. Repository Cleanliness

  • Updated .gitignore to exclude test-generated files:
    • Added pattern to ignore test-generated PNG files (test_*.png)
    • Added specific test output file exclusions while preserving documentation images
  • Replaced hard-coded test output paths with environmentally aware paths

4. Test Reliability

  • Replaced static test output JSON with temporary files in tests/test_routeparams.py
  • Fixed import issues in test files
  • Ensured all tests clean up after themselves

Technical Details

  • Modified 9 test files to address warnings and improve file handling
  • Added warning filters for 6 different types of warnings from external dependencies
  • Created a robust warning management system in conftest.py that can be easily extended
  • Test-generated figures now use the standard project configuration for output paths
  • Updated .gitignore with patterns that match test output files

With these changes, we've eliminated 17 warnings while maintaining all 83 passing tests.

Future Improvements

  • Consider moving all test output to a dedicated temporary test output directory
  • Add automated cleanup of test output files that might accumulate during development
  • Further standardize how tests generate and manage visualization outputs

@kdemmich
Copy link
Collaborator

Thank you for your PR! We appreciate the time and effort that was put into it. This PR contains changes in several context while we are only interested in the introduction of the function validate_parameters in direct_power_boat.py and the corresponding tests as these changes fix Issue #41 If you remove all other changes, we will be happy to merge your PR. For the future, we recommend splitting changes in different contexts in separate commits and PRs.

@TheRGuy9201
Copy link
Author

Surely, I will delete all the unnecessary changes and submit the pull request.

- Add validate_parameters method that checks for dummy values (-99) in ship geometry parameters
- Add call to validate_parameters in calculate_ship_geometry method
- Add call to load_data in __init__ method to ensure validation during initialization
- Fix Issue 52North#41: validate parameters and raise meaningful error messages
@TheRGuy9201
Copy link
Author

TheRGuy9201 commented Jul 16, 2025

Now I have made all the changes like the previous one, keeping only the introduction of the function validate_parameters in direct_power_boat.py and the corresponding tests, for the Issue #41 take a look and let me know if it's complete or if you need anything else.

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