fix: SKILZ-64 and SKILZ-65 Bug Fixes - Temp Directory Warnings & --config Flag #37
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Bug Fixes ### SKILZ-64: Temp Directory Warning During Git Installs - **Issue**: Directory name validation ran on temp git clone directories - **Fix**: Skip validation when git_url is provided (temp directories) - **Result**: No more false warnings during successful git installs ### SKILZ-65: --config Flag Not Working for Git Installs - **Issue**: --config flag ignored for git URL installations - **Fix**: Added config_file parameter to install_from_git() function - **Result**: --config flag now works consistently across all install types ## Testing & Quality Assurance ### TDD Approach Implemented - ✅ **Red Phase**: Created failing E2E and unit tests demonstrating bugs - ✅ **Green Phase**: Implemented fixes, all tests now pass - ✅ **Refactor Phase**: Clean, maintainable code with proper error handling ### Comprehensive Test Suite Added - **E2E Tests**: �[0;34m[INFO]�[0m Starting E2E Tests for SKILZ-64 and SKILZ-65 Bug Fixes �[0;34m[INFO]�[0m These tests should FAIL initially, then PASS after fixes �[0;34m[INFO]�[0m Setting up test environment... �[0;34m[INFO]�[0m Test environment ready: /tmp/skilz-bug-test-1767989240 ========================================== �[0;34m[INFO]�[0m Testing BUG 1: SKILZ-64 - Temp Directory Warnings ========================================== �[0;34m[INFO]�[0m Testing Bug 1: Git installs should not warn about temp directories �[0;32m[PASS]�[0m BUG1_GIT_NO_TEMP_WARNING: No temp directory warnings found - Real-world scenario testing - **Unit Tests**: - Function-level validation - **Regression Tests**: All existing 633 tests still pass - **Live API Testing**: Validates marketplace integration ### Specification Updates - Updated core installer spec to clarify temp directory handling - Updated universal agent spec to include git install --config support - Enhanced documentation with E2E testing procedures ### Documentation Improvements - Updated README.md with E2E testing commands - Enhanced DEPLOY_PYPI.md with comprehensive testing checklist - Added quality assurance section with testing strategy ## Technical Implementation ### Code Changes - : Skip directory validation for git installs - : Added config_file parameter support - : Pass config_file to git installs - : Updated config sync logic for --config flag ### Test Coverage - 4 new E2E test scenarios covering both bugs - 4 new unit tests with proper mocking - All tests validate both positive and negative cases - Tests run in isolated environments, no user config modification ## Success Criteria Met ✅ **SKILZ-64**: Git installs don't show temp directory warnings ✅ **SKILZ-65**: --config flag works for git URL installations ✅ **No Regressions**: All existing functionality preserved ✅ **Test Coverage**: Comprehensive E2E and unit test coverage ✅ **Documentation**: Updated specs and deployment guides ## Quality Metrics - **633 tests passing** (100% success rate) - **4 new E2E tests** validating bug fixes - **4 new unit tests** with proper isolation - **0 regressions** introduced - **Clean code** following existing patterns Closes SKILZ-64, SKILZ-65
- Fix import sorting and formatting - Remove unused pytest import - Fix long line by breaking it up - Ensure all linting checks pass
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🐛 SKILZ-64 and SKILZ-65 Bug Fixes
Overview
Complete fix for two critical bugs affecting Skilz v1.7.0 user experience:
Changes Made
🔧 Code Changes
config_fileparameter toinstall_from_git()function🧪 Testing Added
�[0;34m[INFO]�[0m These tests should FAIL initially, then PASS after fixes
�[0;34m[INFO]�[0m Setting up test environment...
�[0;34m[INFO]�[0m Test environment ready: /tmp/skilz-bug-test-1767989454
�[0;34m[INFO]�[0m Testing BUG 1: SKILZ-64 - Temp Directory Warnings
�[0;34m[INFO]�[0m Testing Bug 1: Git installs should not warn about temp directories
�[0;32m[PASS]�[0m BUG1_GIT_NO_TEMP_WARNING: No temp directory warnings found** - Comprehensive E2E test suite
5. **** - Unit tests with proper mocking
📚 Documentation Updated
Bug Details
SKILZ-64: Temp Directory Warning During Git Installs
Problem: Git installs showed confusing warnings about temp directory names
Fix: Skip directory validation when
git_urlis provided (temp directories)Result: No false warnings during successful git installations
SKILZ-65: --config Flag Not Working for Git Installs
Problem:
--config FILENAME.mdignored for git URL installationsFix: Added config_file parameter support throughout git install pipeline
Result: --config flag works consistently across all install types
Testing Strategy
TDD Approach
Test Coverage
Quality Assurance
Success Criteria Met
Related Issues
Closes SKILZ-64, SKILZ-65
Testing
Run the comprehensive test suite:
Impact