tests(pypi): add tests for fetching package from pypi #39
Conversation
Signed-off-by: Rahul Tiwari <[email protected]>
| @@ -105,7 +105,7 @@ func NewPypiPackageDataFactory(client *helper.Client) PypiPackageDataFactory { | |||
| func (pf *pypiPackageDataFactory) GetPackageData(packageJSONURL string) (PypiPackageData, error) { | |||
| packageInfo := PypiPackageData{} | |||
|
|
|||
| packageJSONURL = strings.Replace(packageJSONURL, "pypi.org", "", 1) | |||
| packageJSONURL = strings.ReplaceAll(packageJSONURL, "pypi.org", "") | |||
There was a problem hiding this comment.
Could you edit your commit to explain why this change is needed?
There was a problem hiding this comment.
Since the http client was changed to accept a baseURL, so that external http requests can be mocked, I had to strip off pypi.org from the base url and hence in the first commit, I had introduced
packageJSONURL = strings.Replace(packageJSONURL, "pypi.org", "", 1)
which was later replaced by just to abide by the code style of the codebase.
packageJSONURL = strings.ReplaceAll(packageJSONURL, "pypi.org", "")
I thought the chore prefix to the commit made sense, but I'm open to adding a comment or changing the commit message for more verbosity.
| } | ||
|
|
||
| return r, nil | ||
| } |
There was a problem hiding this comment.
This is good for now. Thanks!
There was a problem hiding this comment.
Thanks for the PR! It looks to me that there are some significant changes added beyond writing tests. My suggestion is to submit smaller changes and explain in the commit why the change is needed. For example, why should we add a new PyPiSoftwareFactory datatype? What is missing from the current implementation? How does the change help with further changes or the final goal?
| type pypiPackageDataFactory struct { | ||
| client *helper.Client | ||
| } | ||
|
|
||
| client := &http.Client{} | ||
| response, err := client.Do(request) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| type PypiPackageDataFactory interface { | ||
| GetPackageData(packageJSONURL string) (PypiPackageData, error) |
There was a problem hiding this comment.
I'm trying to understand why we need a new interface type. Could you explain?
There was a problem hiding this comment.
The interface was introduced so that all receiver functions could be mocked in tests.
For instance, BuildModule fetches package data using GetPackageData and for writing unit tests for BuildModule it might be necessary to mock the call to GetPackageData.
|
Hi @ba11b0y, it would be much easier to figure out what is happening with the tests failing if your changes were small. That would help a lot to get the changes merged. Thanks for the work you put into this! |
I see the tests are failing on And my apologies for sending in a huge PR to review, I understand it makes the review all the more difficult :/ |
|
Putting this on hold until this is actually needed. |
Since it wasn't easy to test the existing worker code, I've tried to refactor it so that it can be mocked and also added a test for fetching PackageData from PyPi.
@puerco @nishakm if this seems fine, I'll go ahead and write more tests to increase coverage.