- 
                Notifications
    You must be signed in to change notification settings 
- Fork 14
Improve test coverage #462
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: master
Are you sure you want to change the base?
Conversation
| Reviewer's Guide by SourceryThis pull request adds a comprehensive 'about.md' document to the 'notes' directory. This document provides a detailed overview of the VCSPull project, covering its purpose, architecture, configuration, codebase structure, development practices, tooling, and usage. Class Diagram for VCSPull ConfigurationclassDiagram
    class ConfigFile {
        +str path
        +dict repos
    }
    class Repository {
        +str url
        +dict remotes
    }
    class VCSClient {
        +str vcs_type
        +str url
        +sync()
    }
    ConfigFile -- Repository : contains
    Repository -- VCSClient : uses
File-Level Changes
 Tips and commandsInteracting with Sourcery
 Customizing Your ExperienceAccess your dashboard to: 
 Getting Help
 | 
| Codecov ReportAttention: Patch coverage is  
 Additional details and impacted files@@             Coverage Diff             @@
##           master     #462       +/-   ##
===========================================
- Coverage   78.98%   23.92%   -55.07%     
===========================================
  Files           8       10        +2     
  Lines         414     1045      +631     
  Branches       85      150       +65     
===========================================
- Hits          327      250       -77     
- Misses         52      789      +737     
+ Partials       35        6       -29     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
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.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- This is a great overview of the project, but it's unclear how it improves test coverage as the title suggests.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
45d442a    to
    5be857c      
    Compare
  
    02b87ec    to
    3e3654b      
    Compare
  
    5be857c    to
    ac5af18      
    Compare
  
    126baa7    to
    06fdc1f      
    Compare
  
    | # Infer VCS from URL if not already set | ||
| if "vcs" not in repo_data and "url" in repo_data: | ||
| url = repo_data["url"] | ||
| if "github.com" in url or url.endswith(".git"): | 
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
github.com
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 8 months ago
To fix the problem, we need to replace the substring check with a more robust method that parses the URL and checks the hostname. Specifically, we should use the urlparse function from the urllib.parse module to extract the hostname and then check if it matches the expected domain.
- Import the urlparsefunction from theurllib.parsemodule.
- Replace the substring check with a parsed URL check.
- Ensure that the check handles arbitrary subdomain sequences correctly.
- 
    
    
    Copy modified line R14 
- 
    
    
    Copy modified lines R223-R224 
- 
    
    
    Copy modified line R226 
| @@ -13,3 +13,3 @@ | ||
| from typing import Any, Optional | ||
|  | ||
| from urllib.parse import urlparse | ||
| import yaml | ||
| @@ -222,5 +222,6 @@ | ||
| url = repo_data["url"] | ||
| if "github.com" in url or url.endswith(".git"): | ||
| parsed_url = urlparse(url) | ||
| if parsed_url.hostname and (parsed_url.hostname == "github.com" or url.endswith(".git")): | ||
| repo_data["vcs"] = "git" | ||
| elif "bitbucket.org" in url and not url.endswith(".git"): | ||
| elif parsed_url.hostname and parsed_url.hostname == "bitbucket.org" and not url.endswith(".git"): | ||
| repo_data["vcs"] = "hg" | 
| url = repo_data["url"] | ||
| if "github.com" in url or url.endswith(".git"): | ||
| repo_data["vcs"] = "git" | ||
| elif "bitbucket.org" in url and not url.endswith(".git"): | 
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
bitbucket.org
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 8 months ago
To fix the problem, we need to ensure that the URL is properly parsed and validated to confirm that the host is exactly "bitbucket.org" and not just a substring within the URL. We can use the urlparse function from the urllib.parse module to parse the URL and then check the hostname.
- Parse the URL using urlparse.
- Extract the hostname from the parsed URL.
- Check if the hostname is exactly "bitbucket.org".
This change should be made in the migrate_v1_to_v2 function, specifically around the lines where the URL is being checked for "bitbucket.org".
- 
    
    
    Copy modified lines R223-R224 
- 
    
    
    Copy modified line R226 
| @@ -222,5 +222,6 @@ | ||
| url = repo_data["url"] | ||
| if "github.com" in url or url.endswith(".git"): | ||
| parsed_url = urlparse(url) | ||
| if "github.com" in parsed_url.hostname or url.endswith(".git"): | ||
| repo_data["vcs"] = "git" | ||
| elif "bitbucket.org" in url and not url.endswith(".git"): | ||
| elif parsed_url.hostname == "bitbucket.org" and not url.endswith(".git"): | ||
| repo_data["vcs"] = "hg" | 
982b920    to
    87e8c24      
    Compare
  
    87e8c24    to
    98e5e92      
    Compare
  
    98e5e92    to
    f479f5c      
    Compare
  
    …odels why: Enhance test coverage and verification of configuration models through property-based testing, ensuring models behave correctly with a wide variety of inputs beyond specific examples. what: - Implement property-based testing using Hypothesis for configuration models - Create comprehensive test strategies for generating valid URLs, paths, and model instances - Add tests verifying serialization roundtrips and invariant properties - Ensure tests verify Repository, Settings, VCSPullConfig, LockFile, and LockedRepository models - Fix type annotations and linting issues in test files - Add Hypothesis dependency to development dependencies refs: Addresses "Property-Based Testing" item from TODO.md
…n loader why: Enhance test coverage and reliability of the configuration system by implementing property-based testing with Hypothesis and comprehensive integration tests. what: - Created property-based tests for configuration loading, saving, and include resolution - Added test generators for repository URLs, paths, and configuration objects - Implemented integration tests for complete configuration workflow - Fixed circular include detection in resolve_includes to prevent infinite recursion - Added proper tracking of processed paths to avoid duplicated processing - Ensured all code follows project style guidelines and has proper type annotations - Improved test reliability with proper temporary file and directory handling refs: Completes "Property-Based Testing" section in notes/TODO.md
why: Facilitate user transition from old nested config format to new Pydantic v2 format what: - Implement migration module to detect and convert configuration versions - Add CLI command with dry-run, backup, and color output options - Create comprehensive test suite with property-based testing - Write detailed migration guide for users See also: notes/proposals/01-config-format-structure.md
f479f5c    to
    b0522cc      
    Compare
  
    
Summary by Sourcery
Documentation: