Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add unit test for "get_locally_installed_packages" function #390

Closed
wants to merge 1 commit into from
Closed

Add unit test for "get_locally_installed_packages" function #390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/_local_packages/invalid_module/EGG-INFO/top_level.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
invalid_module
2 changes: 2 additions & 0 deletions tests/_local_packages/valid_package-1.0.0.dist-info/top_level.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
valid_package
tests
20 changes: 19 additions & 1 deletion tests/test_pipreqs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import io
import sys
import unittest
from unittest.mock import patch
import sys

Check failure on line 15 in tests/test_pipreqs.py

View workflow job for this annotation

GitHub Actions / Lint 

[flake8] reported by reviewdog 🐶
redefinition of unused 'sys' from line 12

Raw Output:
./tests/test_pipreqs.py:15:1: F811 redefinition of unused 'sys' from line 12
Copy link
Collaborator

Choose a reason for hiding this comment

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

sys is already imported in line 12, 3 lines up haha

import os
import requests

Expand Down Expand Up @@ -48,7 +50,14 @@
os.path.dirname(__file__), "_data_duplicated_deps"
)
self.requirements_path = os.path.join(self.project, "requirements.txt")
self.alt_requirement_path = os.path.join(self.project, "requirements2.txt")
self.alt_requirement_path = os.path.join(
self.project,
"requirements2.txt"
)
self.local_packages_path = os.path.join(
os.path.dirname(__file__),
"_local_packages"
)

def test_get_all_imports(self):
imports = pipreqs.get_all_imports(self.project)
Expand Down Expand Up @@ -109,6 +118,15 @@
for item in imports_with_info:
self.assertTrue(item["name"].lower() in self.local)

def test_get_locally_installed_packages(self):
with patch.object(sys, 'path', new=[self.local_packages_path]):
local_installed_packages = pipreqs.get_locally_installed_packages()
expected_output = [
{'exports': ['valid_package'], 'name': 'valid_package', 'version': '1.0.0'},
{'exports': [], 'name': 'EGG', 'version': 'INFO'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, I dont understand this test. Could you explain it to me? why this becomes the output and why is the module invalid? Is it because it haz an additional folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

First of all, we based our tests on our understanding of the code. We don't know exactly how pip builds the structure of a package.

  1. The first test case was not necessary, so we decided to remove it.
  2. The second one was called as a "valid_package" because its "top_level.txt" file has one module named "valid_package" that is not in our code's blacklist (which is a variable called ignore with value ["tests", "_tests", "egg", "EGG", "info"]) and also has a module named "tests" that is in that blacklist.
  3. The last test case makes sure that "top_level.txt" files that are inside a folder that starts with "EGG" are not considered.

We tried to understand the code's behavior by reading "setuptools" documentation (link), but we did not understand much. It requires a lot of reading and time for understanding.

If you think it's okay for us to use our time to study this and then validate if it's right, that is okay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaborate a bit more as to why the first test was unnecessary?

We tried to understand the code's behavior by reading "setuptools" documentation (link), but we did not understand much. It requires a lot of reading and time for understanding.

If you think it's okay for us to use our time to study this and then validate if it's right, that is okay.

I see, I wasn´t expecting it to be that convoluted. I will get back to you as to what we should do, for now just fix the double import and focus on the other tasks

]
self.assertEqual(local_installed_packages, expected_output)

def test_init(self):
"""
Test that all modules we will test upon are in requirements file
Expand Down
Loading