Skip to content

Added support for more test methods, including Yarn and Composer #71

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

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Added support for more test methods, including Yarn and Composer #71

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
4 changes: 3 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,12 @@ As well as testing individual packages you can also test all packages found in v

* `test_pipfile(<file-handle-or-string>)` - returns an IssueSet for all Python dependencies in a `Pipfile`
* `test_gemfilelock(<file-handle-or-string>)` - returns an IssueSet for all Ruby dependencies in a `Gemfile`
* `test_packagejson(<file-handle-or-string>)` - returns an IssueSet for all Javascript dependencies in a `package.json` file
* `test_packagejson(<file-handle-or-string>, (<lock-file-handle-or-string>))` - returns an IssueSet for all Javascript dependencies in a `package.json` file. Optionally takes a `package.lock` file
* `test_gradlefile(<file-handle-or-string>)` - returns an IssueSet for all dependencies in a `Gradlefile`
* `test_sbt(<file-handle-or-string>)` - returns an IssueSet for all dependencies defined in a `.sbt` file
* `test_pom(<file-handle-or-string>)` - returns an IssueSet for all dependencies in a Maven `pom.xml` file
* `test_yarn(<file-handle-or-string>, <lock-file-handle-or-string>)` - returns an IssueSet for all dependencies in Yarn `package.json` and `yarn.lock` files
* `test_composer(<file-handle-or-string>, <lock-file-handle-or-string>)` - returns an IssueSet for all dependencies in Composer `composer.json` and `composer.lock` files

For example, here we are testing a Python `Pipfile`.

Expand Down
23 changes: 20 additions & 3 deletions snyk/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def notification_settings(self):
def invite(self, email: str, admin: bool = False):
raise SnykNotImplementedError # pragma: no cover

def _test(self, path, contents=None):
def _test(self, path, contents=None, additional=None):
Copy link

Choose a reason for hiding this comment

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

How about we add type hints for this method as well, like so:

def _test(self, path: str, contents: typing.IO = None, additional: typing.IO = None)

using typing.IO will cover both binary(if necessary) and text file-like objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This won't quite work as is, as the interface accepts file handles or strings.

Copy link

Choose a reason for hiding this comment

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

Thanks for clarifying, API looks ambiguous to me. shouldn't the function calling _test be the one reading the file content and passing that on? I guess this is an inherited design smell.

worth a re-factor, though I'm sure we'll have to consider backwards compatibility here.

for instance why test_pipfile(self, content) is acting like test_pipfile_content(self, content) while what I think would be cleaner is:

"""  test_pipfile tests a pip file for issue """
def test_pipfile(self, pip_file: typing.TextIO ) -> bool:
    with open(pip_file) as f:
        self._test(contents=f.read())

if contents:
# Check for a file-like object, allows us to support files
# and strings in the same interface
Expand All @@ -176,6 +176,15 @@ def _test(self, path, contents=None):
"encoding": "base64",
"files": {"target": {"contents": encoded}},
}

# Some test methods carry a second file, often a lock file
if additional:
Copy link

Choose a reason for hiding this comment

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

Python is EAFP than LBYL therefore a Pythonic way of writing this code block would look like:

try:
    read = getattr(additional, "read", None)
    additional = additional.read()
    encoded = base64.b64encode(additional.encode()).decode()
    post_body["files"]["additional"] = {"contents": encoded}
except:
    pass

I'm aware of the fact that the code block at the top of this method is LBYL and therefore this one is too, but worth noting IMHO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I refer the consistency vs having two stanzas with different styles. Not against someone refactoring the whole method mind. It requires some additional type hints I think.

Copy link

Choose a reason for hiding this comment

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

You have a point there. let's take this one separately as part of a refactor (happy to take it)

read = getattr(additional, "read", None)
if callable(read):
additional = additional.read()
encoded = base64.b64encode(additional.encode()).decode()
post_body["files"]["additional"] = {"contents": encoded}

resp = self.client.post(path, post_body)
else:
resp = self.client.get(path)
Expand Down Expand Up @@ -212,9 +221,9 @@ def test_gemfilelock(self, contents):
path = "test/rubygems?org=%s" % self.id
return self._test(path, contents)

def test_packagejson(self, contents):
def test_packagejson(self, contents, lock=None):
path = "test/npm?org=%s" % self.id
return self._test(path, contents)
return self._test(path, contents, lock)

def test_gradlefile(self, contents):
path = "test/gradle?org=%s" % self.id
Expand All @@ -228,6 +237,14 @@ def test_pom(self, contents):
path = "test/maven?org=%s" % self.id
return self._test(path, contents)

def test_composer(self, contents, lock):
path = "test/composer?org=%s" % self.id
return self._test(path, contents, lock)

def test_yarn(self, contents, lock):
path = "test/yarn?org=%s" % self.id
return self._test(path, contents, lock)


@dataclass
class Integration(DataClassJSONMixin):
Expand Down
21 changes: 21 additions & 0 deletions snyk/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,13 @@ def test_packagejson_test_with_file(
requests_mock.post("%s/test/npm" % base_url, json=blank_test)
assert organization.test_packagejson(fake_file)

def test_packagejson_test_with_files(
self, organization, base_url, blank_test, fake_file, requests_mock
):

requests_mock.post("%s/test/npm" % base_url, json=blank_test)
assert organization.test_packagejson(fake_file, fake_file)

def test_gradlefile_test_with_file(
self, organization, base_url, blank_test, fake_file, requests_mock
):
Expand All @@ -160,6 +167,20 @@ def test_pom_test_with_file(
requests_mock.post("%s/test/maven" % base_url, json=blank_test)
assert organization.test_pom(fake_file)

def test_composer_with_files(
self, organization, base_url, blank_test, fake_file, requests_mock
):

requests_mock.post("%s/test/composer" % base_url, json=blank_test)
assert organization.test_composer(fake_file, fake_file)

def test_yarn_with_files(
self, organization, base_url, blank_test, fake_file, requests_mock
):

requests_mock.post("%s/test/yarn" % base_url, json=blank_test)
assert organization.test_yarn(fake_file, fake_file)

def test_missing_package_test(self, organization, base_url, requests_mock):
requests_mock.get("%s/test/rubygems/puppet/4.0.0" % base_url, status_code=404)
with pytest.raises(SnykError):
Expand Down