cloudstack-pull-rats #311 SUCCESS
This pull request looks good

cloudstack-pull-requests #1007 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-rats #312 SUCCESS
This pull request looks good

cloudstack-pull-requests #1008 SUCCESS
This pull request looks good

cloudstack-pull-analysis #243 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-analysis #244 SUCCESS
This pull request looks good

@wilderrodrigues this one you should see, thanks for the hard work @cristofolini.

LGTM. Please squash the commits before merge. Thanks

@cristofolini Thanks! As @koushik-das mentions, please squash the commits and force-push to your PR branch.

@DaanHoogland Currently @wilderrodrigues is on holiday, he will be back next week.

I know, but he has been busy on this as well, are we in a hurry? On squashing: I agree with the observation that this is one of those exceptions where squashing indeed makes sense. The two actual structural changes (the first two commits) seem identical. The rest are fixes of the changed code.

Hi all, I tried squashing @cristofolini, but for some odd reason could not get the last two together. I had to reset to master and manually add his commits.
It was my bad the sequence of commits, hi is working with me and I forgot to review his code before the commit and push to github. I guess that now everything is ok.

@rafaelweingartner @cristofolini I love your work, especially the fact the so many litle unit tests were added. It has some checkstyle errors unfortunately (in the form of trailing whitspace.

Now, I am having troubles again with squash.

I am using this command “git rebase -i HEAD~2” to squash the last to commits into one. The git prompts me a screen in which I can change commits comments and return “Successfully rebased and updated”. Then, I try to push using “git push –f” to force the remote repository to accept my push. The git answers “Everything up-to-date” , however commits are not getting squashed into remote.
Am I doing something wrong?

@rafaelweingartner if you git rebase -i HEAD~2 and have $EDITOR set to something usable you will get and editor with instructions something like:

pick fa7f8cf @rafaelweingartner     Had a problem while squashing @cristofolini commits, for some odd reason
pick 3937e7b    removed trailing whitespace

# Rebase #######..######## onto ####### (       2 TODO item(s))
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

On the second line replace pick with f or s. if you choose s you get the change to change the commit message, which in this case I would change to "Removed duplicate code in CitrixResourceBase and subclasses". If you do use s a second editor will come.

Holly S...
I am so sorry, when I got that screen with the text I was not even reading it, I thought it was just the comments on each commit, and that git was giving me the opportunity to change it. I did not realize that I had to say what it should to commits.

Now, everything seems ok.

np, but the commit message is now not very clear and it seems you did not really squash them. There are still two commits in this PR.

Fixed the commit title and message.

Is it appearing as two (2) commits for you?
Here it shows as just one.

@rafaelweingartner it seems fine now, let's await checks and merge

cloudstack-pull-rats #324 ABORTED

cloudstack-pull-requests #1020 ABORTED

cloudstack-pull-analysis #256 SUCCESS
This pull request looks good

cloudstack-pull-rats #328 ABORTED

cloudstack-pull-rats #331 ABORTED

cloudstack-pull-requests #1026 ABORTED

cloudstack-pull-rats #332 ABORTED

cloudstack-pull-requests #1027 ABORTED

cloudstack-pull-analysis #260 ABORTED

cloudstack-pull-analysis #263 ABORTED

cloudstack-pull-analysis #264 ABORTED

@cristofolini @DaanHoogland Are these unit tests adding any value? What type of value are they adding here? To me these seems to be burden on maintainer or developer who is going to touch this code in future.

if somebody in future wants to change path then only he will change the method and this unit test will fail which will make him to change according to his requirements. In that scenario purpose of tests is altogether lost. Unit tests I believe are not meant to be modified.

@anshul1886 if the purpose is to ensure that the path returned doesn't change, this seems perfectly aceptable to me. If people want to change the path in the future they must be aware and change the unit tests as well. That said they seem a litle trivial but I'm alright with that. It is mainly the integrity class hierarchy of the resources that is being tested this way.
(comment moved to this PR where it makes less non-sense)

@DaanHoogland That doesn't seems to be the case here. I believe if somebody has to change unit test with every change then that unit test is burden and should not be kept in codebase. I still didn't get the real purpose here as your statement starts with if.

@anshul1886 I agree with your point about the purpose of unit tests. In this case the purpose is to make sure developers don't change the paths, isn't it? So that when changes to the resource hierarchy occur paths for specific versions don't change. It makes sense to me. What changes do you envision that would justify changing the paths and would make those tests a burden?

Hi all,
Sorry the delay.

Well, @anshul1886 IMHO the test cases add value to the code, they are making sure that those paths are properly coded (that may be too simple; however in such a huge, complex and complicated code such as the ACS they are needed). We try to use TDD to develop anything we are doing here. Therefore, when we were making those changes we did the following:

• Created an abstract method: com.cloud.hypervisor.xenserver.resource.CitrixResourceBase.getPatchFilePath()
• Then we just generated that method in CitrixResourceBase subclasses
• Extracted the body of the method “getPatchFiles” from CitrixResourceBase subclasses
• At that moment we already had the methods we needed, so we could start creating the tests to check if each instance was returning the expected path. Every single test was failing at this moment as we expected.
• Then we started coding the methods in each class that was needed. After that we ran the tests to check if everything was ok.

I believe those tests add value and that they are not a burden as pointed out by @DaanHoogland , if some future commit comes to touch that code and change the paths by mistake our test cases will get that.

Additionally, I also believe that tests cases should be as simple as that, if the test method is too complicated there is something wrong either with the code or with the coder.

But at the end if you guys think that they are not necessary, I can remove them. No hard feelings ;)

@rafaelweingartner @DaanHoogland If the purpose is to make sure that path is not modified by other developer then adding note/comment on top of that line makes more sense. Even adding note is kind of implicit as paths are kind of constants which any developer would think before changing. Tests are not meant for that purpose.

@rafaelweingartner I like it! For me it simply proves that the paths are still the same after your change. If the paths need to be changed, we'll update the tests and the code. As you'd do with any code you change that has tests.

@rafaelweingartner I think it's good that you wrote the unit tests, and I actually wished people were a bit more supportive. I strongly agree with @remibergsma. I think we should merge it...

LGTM

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 @rafaelweingartner worked with that to make the rest better.

I would merge this now, because it is good work, and later (new PR) 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.

Edit: give credits to the committer (not sure if @cristofolini was also involved in this, but if that is the case credits to him too 😄)

Thanks all for your efforts, I'm merging this now.