cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From miguelaferreira <>
Subject [GitHub] cloudstack pull request: Notify listeners when a host has been add...
Date Mon, 14 Sep 2015 13:34:58 GMT
Github user miguelaferreira commented on the pull request:
    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 or file a JIRA ticket
with INFRA.

View raw message