-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/test branch #1
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
Conversation
Reviewer's Guide by SourceryThis PR introduces significant improvements to the virtual environment creation codebase, focusing on type safety, error handling, and test coverage. The changes include adding type hints throughout the code, improving error handling with better feedback, and implementing comprehensive unit tests. Updated class diagram for VenvCreatorclassDiagram
class VenvCreator {
-os_type: str
-is_windows: bool
-is_linux: bool
-python_installations: List~str~
-username: str
-dependencies_folder: Optional~str~
+__init__()
+set_dependencies_folder(folder_path: str) bool
+find_dependencies() List~str~
+install_local_dependencies(venv_path: str) Optional~bool~
+get_current_username() Optional~str~
+_get_pip_path(venv_path: str) str
+find_python_installations() List~str~
+_find_windows_python() List~str~
+_find_linux_python() List~str~
+get_python_version(python_path: str) str
+create_venv(python_path: str, venv_path: str) bool
+install_requirements(venv_path: str, requirements_path: Optional~str~) Optional~bool~
+get_project_details() Dict~str, str~
+print_activation_instructions(project_path: str, venv_name: str) void
+run() void
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @selectdimensions - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please update the PR title and description to better reflect the changes (adding type hints, improving error handling, expanding tests)
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
test_venv_creator.py
Outdated
class TestVenvCreator(unittest.TestCase): |
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.
issue (complexity): Consider simplifying test implementations by reducing helper methods and nesting levels
The test implementation could be simplified while maintaining the same coverage. Here are specific suggestions:
- Simplify the project details test by using a context manager approach:
def test_get_project_details(self):
"""Test getting project details"""
with patch('builtins.input', side_effect=['test_project', self.temp_dir, 'venv', 'n', 'n']):
details = self.venv_creator.get_project_details()
self.assertEqual(details['name'], 'test_project')
self.assertEqual(details['path'], self.temp_dir)
- Remove the create_test_wheel helper and inline the wheel creation where needed:
def test_find_dependencies(self):
"""Test finding dependency files"""
# Create test files directly
wheel_path = os.path.join(self.test_dependencies, "package1-1.0-py3-none-any.whl")
with open(wheel_path, 'w') as f:
f.write('dummy wheel file')
self.venv_creator.set_dependencies_folder(self.test_dependencies)
dependencies = self.venv_creator.find_dependencies()
self.assertIn(wheel_path, dependencies)
- Simplify the install_local_dependencies test by reducing setup layers:
@patch('subprocess.run')
def test_install_local_dependencies(self, mock_run):
"""Test installing local dependencies"""
mock_run.return_value.returncode = 0
venv_path = os.path.join(self.temp_dir, 'venv')
self.venv_creator.set_dependencies_folder(self.test_dependencies)
self.assertTrue(self.venv_creator.install_local_dependencies(venv_path))
mock_run.assert_called()
These changes maintain test coverage while reducing nesting and complexity.
test_venv_creator.py
Outdated
if installations: | ||
self.assertTrue(all(os.path.exists(path) for path in installations)) |
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.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
create_venv.py
Outdated
"""Get project details from user""" | ||
project_details = {} | ||
project_details: Dict[str, str] = {} |
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.
issue (code-quality): We've found these issues:
- Merge dictionary assignment with declaration (
merge-dict-assign
) - Use named expression to simplify assignment and conditional [×2] (
use-named-expression
) - Lift code into else after jump in control flow (
reintroduce-else
) - Swap if/else branches (
swap-if-else-branches
)
Lets test what we have arranged
Summary by Sourcery
Enhance the VenvCreator class with type annotations and refactor its methods for better readability and maintainability. Update the CI workflow to include additional testing and linting steps, and convert tests to the unittest framework with expanded coverage.
New Features:
Enhancements:
CI:
Tests: