Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140077337
Hi again Mike,
Thanks for addressing my questions.
Regarding the commented-out code, I see from your answers that the reason
for leaving commented-out code was to make it easy to revert it back in the
future.
I also see that you did not follow the same reasoning for other code
changes you made in this same PR. And I argue that those changes are also very
easy to revert. For that we need only to leverage the version control system.
It is easy to track the changes of a given file, and recover it's previous
states.
May I recommend that you remove the commented out code, since we still keep
the possibility to easily revert those changes in the future?
Regarding the automated testing, I'm very happy to hear that you are
working on integration test. I assume from your answer that those integration
tests are not committed in the git repository, and that is something I would
expect to see happen. Please do correct me if my assumption is wrong.
I trust you when you say that "there is essentially no existing test code"
covering the code you changed. I wouldn't go as far as to call that fortunate.
I actually think it is a very sad truth.
I think we should unit-test as much as we can, and especially targeting
code that has no coverage at all.
I'm sure that given the amount of changes made in this PR there is ample
room for covering at least some of it with unit-tests.
Fact is, that if we continue to add code without the complementary
unit-tests, our test coverage drops. Is that something we want?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
