Skip to content

Conversation

@fclairamb
Copy link
Owner

Summary

This PR fixes and enhances the PASV port mapping functionality introduced in PR #521, addressing test coverage issues and fixing a bug in the NumberAttempts() calculation.

Changes Made

🐛 Bug Fixes

  • Fixed PortRange.NumberAttempts() calculation: Changed from r.End - r.Start to r.End - r.Start + 1 to correctly include both endpoints in the range
  • Improved port range validation: Ensures proper port range handling for both PortRange and PortMappingRange

✅ Test Coverage Improvements

  • Added TestPortMappingRange: Comprehensive test for the new PortMappingRange functionality
    • Tests FetchNext() method returns correct exposed and listened ports
    • Tests NumberAttempts() method returns correct count
    • Tests integration with FTP server using both PASV and EPSV commands
    • Validates that exposed ports are in the correct range
  • Added TestPortRangeBackwardCompatibility: Ensures existing PortRange functionality continues to work
    • Tests backward compatibility with existing code
    • Validates that PortRange correctly implements PasvPortGetter interface
  • Added getPortFromEPSVResponse() helper: Utility function to parse EPSV command responses for testing

🔧 Code Quality

  • Enhanced test coverage: Addresses the codecov issues mentioned in PR feat: support pasv mode port mapping #521
  • Comprehensive validation: Tests both PASV and EPSV commands with port mapping
  • Backward compatibility: Ensures existing PortRange usage remains unaffected

Related Issues

This PR addresses the test coverage issues in PR #521 which implements support for passive mode port mapping. The original PR introduced the PasvPortGetter interface and PortMappingRange struct to support Docker deployments where port mapping exists between client and server.

Testing

All tests pass, including:

  • Existing functionality tests (backward compatibility)
  • New port mapping functionality tests
  • Integration tests with both PASV and EPSV commands
  • Edge case validation
go test -v ./... -skip "TestTransferIPv6"
# All tests pass except TestTransferIPv6 (expected to fail in environments without IPv6 support)

Benefits

  1. Docker Support: Enables FTP server deployment in Docker containers with port mapping
  2. Backward Compatibility: Existing PortRange usage continues to work unchanged
  3. Comprehensive Testing: Improved test coverage for passive transfer functionality
  4. Bug Fixes: Corrected port range calculation issues

Breaking Changes

None. This PR maintains full backward compatibility while adding new functionality.

KirCute and others added 4 commits February 27, 2025 14:45
…Attempts calculation

- Add TestPortMappingRange to test the new PortMappingRange functionality
- Add TestPortRangeBackwardCompatibility to ensure backward compatibility
- Add getPortFromEPSVResponse helper function for EPSV response parsing
- Test both PASV and EPSV commands with port mapping
- Fix PortRange.NumberAttempts() to return correct count (End - Start + 1)
- Improve test coverage for the new PasvPortGetter interface
@codecov
Copy link

codecov bot commented Jun 5, 2025

Codecov Report

Attention: Patch coverage is 84.37500% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.77%. Comparing base (d4ec1a1) to head (067aa30).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
transfer_pasv.go 75.00% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
+ Coverage   86.68%   86.77%   +0.09%     
==========================================
  Files          11       12       +1     
  Lines        2328     2344      +16     
==========================================
+ Hits         2018     2034      +16     
+ Misses        236      235       -1     
- Partials       74       75       +1     

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

cursor[bot]

This comment was marked as outdated.

- Run gofmt and goimports to ensure consistent code formatting
- All tests continue to pass
- Code compiles successfully
@openhands-ai
Copy link

openhands-ai bot commented Jun 5, 2025

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Build

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #540

Feel free to include any additional details that might help me get this PR into a better state.

You can manage your notification settings

cursor[bot]

This comment was marked as outdated.

- Add proper comments for exported methods
- Fix gosec warnings with nolint directives for acceptable weak random usage
- Fix forcetypeassert by using proper type assertion checks
- Fix varnamelen issues by using descriptive variable names
- Fix nlreturn issues by adding blank lines before returns
- Remove unused nolint directives
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ BugBot reviewed your changes and found no bugs!


BugBot free trial expires on June 9, 2025
Your team has used $0.00 of the $50.00 spend limit so far. Team admins can manage spend limit in the Cursor dashboard.

Was this report helpful? Give feedback by reacting with 👍 or 👎

@fclairamb fclairamb marked this pull request as ready for review June 5, 2025 07:11
@fclairamb fclairamb merged commit cab9a5c into main Jun 5, 2025
4 of 5 checks passed
@fclairamb fclairamb deleted the fix/pasv-port-mapping-tests branch June 5, 2025 07:12
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.

4 participants