-
Notifications
You must be signed in to change notification settings - Fork 2
Add comprehensive testing and improvements for telegram filesystem #6
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
base: main
Are you sure you want to change the base?
Conversation
…ystem - Add telegram_test.go with 640 lines of unit tests covering: * Fake filesystem operations and concurrency * File read/write operations with thread safety * File extension matching * Size limits and validation * Error conditions and edge cases - Improve error handling: * Use os.ErrNotExist consistently in Stat/LstatIfPossible * Add ErrFileTooLarge for size limit violations * Add UTF-8 validation for text files - Add safety features: * File size validation (50MB Telegram limit) * Message size constant (4096 chars) * Directory size constant (4096 bytes) * Mutex protection for File read/write operations - Code cleanup: * Remove commented-out code in fake_fs.go * Add security warnings to bot command handlers * Improve imports organization * Add graceful bot shutdown with Stop() method
Test Files Created:
1. test_helpers.go (210 lines) - Test infrastructure including:
- testLogger - Full logging implementation for tests
- MockBot - Mock Telegram bot for testing (prepared for future use)
- Helper functions: createTestBot(), createTestAccess(), createTestAccessWithToken()
2. telegram_integration_test.go (392 lines) - Integration tests with 13 new test functions:
- LoadFs validation (empty token, invalid ChatID)
- File extension detection (images, videos, audio, text, documents)
- File size validation and limits
- Text size limit enforcement
- Filesystem operations
- Multiple file creation
- File read/write integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive testing infrastructure and improvements to the telegram filesystem backend, achieving ~81% code coverage for production code. It introduces robust testing capabilities using offline bot mode and in-memory fake filesystem operations, while also improving error handling, safety features, and code organization.
- Implements comprehensive testing infrastructure with 35 unit and integration tests
- Adds safety features including file size validation and UTF-8 validation for text files
- Improves error handling with consistent
os.ErrNotExistusage and proper error wrapping - Introduces graceful bot shutdown mechanism and thread-safe file operations
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test_helpers.go | New test infrastructure with mock logger and bot implementations |
| telegram_test.go | New comprehensive unit tests covering filesystem operations and concurrency |
| telegram_integration_test.go | New integration tests for parameter validation and file type detection |
| telegram.go | Core improvements including safety features, error handling, and thread safety |
| file_info.go | Minor improvement replacing magic number with named constant |
| fake_fs.go | Code cleanup removing dead code comments |
| .github/workflows/build.yml | CI cleanup removing unnecessary apt-get command |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Add sync.Once to Fs struct to prevent double-close panic - Update Stop() method to use sync.Once.Do() - Add documentation that Stop() is safe to call multiple times - Add TestFsStopMultipleCalls to verify thread-safe behavior - Test verifies concurrent calls to Stop() don't panic This fixes a potential race condition where calling Stop() multiple times would cause a panic when trying to close an already-closed channel.
Additional Fix: Race Condition in Stop() MethodAdded protection against multiple calls to Changes:
Why This Matters:Calling Test Results: ✅ All 36 tests passing with race detector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
fs/telegram/telegram.go:1
- Add comments explaining these are Telegram's API limits to provide context for why these specific values are used.
// Package telegram provides a telegram access layer
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Change from byte length (len()) to character count (utf8.RuneCountInString()) - Telegram's 4096 limit is characters/runes, not bytes - Add TestTextSizeLimitUTF8 to verify emoji, Cyrillic, ASCII handling - Update maxTextSize documentation to clarify it's character limit This fix allows: - Emoji text up to 16KB in bytes (4096 chars × 4 bytes) - Cyrillic text up to 8KB in bytes (4096 chars × 2 bytes) - ASCII text up to 4KB in bytes (4096 chars × 1 byte) Previously, non-ASCII text was incorrectly rejected when it exceeded 4096 bytes, even if it was under 4096 characters.
Add comprehensive end-to-end testing setup: - e2e-test.sh: Automated test script that builds server, creates test files (PNG, TXT, MD, MP4, JSON), uploads via FTP, and verifies delivery - E2E-TESTING.md: Complete documentation with prerequisites, setup instructions, and troubleshooting guide - TESTING_PROPOSAL.md: Testing strategy documentation - .gitignore: Exclude test artifacts and binaries The E2E script includes: - Prerequisites checking (lftp, config validation) - Automatic server lifecycle management - Multiple file type testing - Manual verification workflow - Robust cleanup with trap handlers - Colorful status output Usage: ./fs/telegram/e2e-test.sh
Summary
This PR adds comprehensive testing infrastructure and improvements to the telegram filesystem backend, achieving ~81% code coverage for production code.
Changes
🧪 Testing Infrastructure (New Files)
test_helpers.go (210 lines)
testLogger: Full logging implementation for tests with thread-safe log collectionMockBot: Mock Telegram bot interface (prepared for future mocking strategies)createTestBot(),createTestAccess(),createTestAccessWithToken()telegram_test.go (640 lines)
telegram_integration_test.go (392 lines)
🔧 Code Improvements
Error Handling
os.ErrNotExistconsistently inStat()andLstatIfPossible()ErrFileTooLargeerror for size limit violationsSafety Features
Write()maxFileSize,maxTextSize,defaultDirSizeStop()method and channel signalingCode Cleanup
fake_fs.go🚀 CI/CD Improvement
.github/workflows/build.ymlby removing unnecessaryapt-getcommandTest Results
Coverage
Coverage Breakdown
Note: Uncovered code (~19%) consists of:
These are intentionally excluded from unit tests and should be tested manually or via E2E tests.
Testing Strategy
The tests follow a pragmatic approach using:
This achieves high coverage without requiring actual Telegram credentials or network access.
Documentation
TESTING_PROPOSAL.mdwith comprehensive testing strategy documentation (available locally, not committed to avoid clutter)Breaking Changes
None. All changes are backward compatible.
Related Issues
Fixes the panic issue from commit a202228 and improves overall code quality and testability.