Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132949485
LGTM
This PR is a great example of how to do code refactoring. Write unit tests
that exercise the semantics, change the logic around it, run the unit tests
again to show the semantics is the same.
Great work.
However, @anshul1886 has a point related to the tests being coupled to the
implementation via the path string. IMHO that is not a result of having the
tests, that is a result of having the path string hardcoded in the class. That
was there already and @cristofolini worked with that to make the rest better.
So I would merge this now, because it is good work, and refactor the class
XcpOssResource (and respective test) to make it parametric on the path string.
While putting this specific string in a constants class, or in a config
attribute (maybe in the DB, not sure about that), to make it easier to change
and reuse.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
