Add unit test for "get_locally_installed_packages" function #390
Conversation
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #390 +/- ##
==========================================
+ Coverage 86.71% 86.97% +0.25%
==========================================
Files 2 2
Lines 256 261 +5
==========================================
+ Hits 222 227 +5
Misses 34 34
☔ View full report in Codecov by Sentry. |
| expected_output = [ | ||
| {'exports': [], 'name': 'invalid_package', 'version': None}, | ||
| {'exports': ['valid_package'], 'name': 'valid_package', 'version': '1.0.0'}, | ||
| {'exports': [], 'name': 'EGG', 'version': 'INFO'} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
- The first test case was not necessary, so we decided to remove it.
- 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. - 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.
There was a problem hiding this comment.
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
|
Also, you should push to next, not master |
|
@willianrocha could you take a look at PR #384 also? It seems related to this test, I would like your opinion about the issue, are non top_level pip packages something compliant with how pip works and the documentation? if so it seems one of the test cases implemented in this test could be a problem |
mateuslatrova
force-pushed
the
get_locally_installed_packages
branch
from
ce27276 to
3876191
Compare
@alan-barzilay Packages that are in the "EGG" format have the "top_level.txt" file, but others don't. For example, pandas is a package in "dist.info" format and it does not have the "top_level.txt" file, but it has the "METADATA" file mentioned in the PR you just said. I don't know which one is compliant or not, but it seems to me that maybe our code is not getting all information it is looking for... In the case of pandas, for example, it would not get the package from the "top_level.txt" and would continue running... Also, in the EGG format, the "PKG-INFO" file seems a lot like the "METADATA" file from "DIST-INFO" format. Obs.: it looks like EGG packages are deprecated... Maybe we could handle both types of distribution... |
| @@ -11,6 +11,8 @@ | |||
| import io | |||
| import sys | |||
| import unittest | |||
| from unittest.mock import patch | |||
| import sys | |||
There was a problem hiding this comment.
sys is already imported in line 12, 3 lines up haha
| expected_output = [ | ||
| {'exports': [], 'name': 'invalid_package', 'version': None}, | ||
| {'exports': ['valid_package'], 'name': 'valid_package', 'version': '1.0.0'}, | ||
| {'exports': [], 'name': 'EGG', 'version': 'INFO'} |
There was a problem hiding this comment.
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
alan-barzilay
force-pushed
the
next
branch
3 times, most recently
from
07a4a64 to
64fc5a2
Compare
validating ignore module and packages