-
Notifications
You must be signed in to change notification settings - Fork 45
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
Test GithubDependencyTreeTask #543
Test GithubDependencyTreeTask #543
Conversation
@jpopelka Your image is available in the registry: |
'com.google.guava:guava:20.0', | ||
'junit:junit:4.10'] | ||
for dep in expected_direct_dependencies + expected_transitive_dependencies: | ||
assert dep in results['dependencies'] |
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.
Storing the results in a set
would result in constant time lookup. Maybe use something like obtained_dependencies = set(results.get('dependencies', []))
?
46272a9
to
9c0efed
Compare
@jpopelka Your image is available in the registry: |
expected_transitive_dependencies = {'commons-io:commons-io:2.0.1', | ||
'com.google.guava:guava:20.0', | ||
'junit:junit:4.10'} | ||
assert expected_transitive_dependencies.issubset(obtained_dependencies) |
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.
I think obtained dependencies should be the exact same as expected_direct_dependencies + expected_transitive_dependencies
. It can look something like this:
expected_dependencies = expected_direct_dependencies.union(expected_transitive_dependencies)
assertSetEqual(obtained_dependencies, expected_dependencies)
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.
We don't need to test whether maven-dependency-plugin
works correctly,
we just need to know whether the task runs it and the test as it is answers that sufficiently.
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.
LGTM. Thanks @jpopelka for writing the tests :)
@jpopelka I have approved the changes. If you can resolve merge conflicts, I can merge it as well. Also there is a failing check for style. |
@jpopelka good to know that the new CI job works as expected ;) |
9c0efed
to
84023ee
Compare
@jpopelka Your image is available in the registry: |
test for #536