Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/700
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132957093
Thanks all for your efforts, I'm merging this now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
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
Manual assertions are not in play. These are not in the tests. Regular
assertEquals calls are made.
Investigating the hierarchy once again to give you a reference I found one
thing that is less then optimal with them. The tests are in testclasses not
generic to the tested resources: XcpOssResource
Please mention lines in tests which is justifying your statement "They prove
that the fragile integrity of the class hierarchy that has been meddled with so
often is still intact”
I have no problem with solution. I have problem with tests.
For reference see section Superficial Test Coverage @
Github user snuf commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132653773
@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
Anshul,
I do not think a reference for the intricate problems we face with the
hierarchy of the CitrixResourceBase and descendants is fair to ask. I think
the burdon of proof is with you and not Rafael.
The tests do not just prove that the assignment works as in you example.
They prove that the fr
@anshul1886 I agree that we disagree.
Folks I do not know what to do in this case, I will not looking for
references to support what I said and did. Whatever you guys decided I will
do (remove or let the test cases there).
On Wed, Aug 19, 2015 at 9:47 AM, Anshul Gangwar
wrote:
> Let me summaris
Let me summarise the tests
class A {
x(){
return “d”; a constant
}
}
test
p=“d”
Assert( A.x() = p)
Which can be reduced to
class A {
q=“d";
}
Here q is replacement for x method as it is only returning a constant
now test is
p=“d"
assert (p=q)
To me this basically proves that java assignment
Can you point to any reference/blog which justifies writing tests for this kind
of scenario?
What I can infer from these tests is that that there are two scenarios
1) Method will not change
In that case it doesn’t make sense to put test for never changing method.
2) Method will change
In
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132550029
@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 test
@anshul1886 I totally agree with you that tests are meant to test
individual, and as you pointed the individual code that we want to test is
“getPatchFilePath”. However, that method is abstract, and its
“implementation” that is as simple as returning a constant, changes in few
subclasses of CitrixR
a unit can be defined at more then just the method level and in this case
those paths have changed from under us in the past. I am not justifying
testing any constant this way. I am justifying just this work.
On Wed, Aug 19, 2015 at 9:03 AM, Anshul Gangwar
wrote:
> What I mean to say is that uni
What I mean to say is that unit test are meant to test individual unit which
here is getPatchFilePath and not meant to test hierarchy as you are pointing
out here. By individual unit I mean it doesn’t matter for test that it is in
class A or class B. This way you are kind of justifying that we s
On Wed, Aug 19, 2015 at 5:56 AM, anshul1886 wrote:
> 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
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132438972
@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 m
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132197866
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
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132189625
@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 th
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132186831
@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
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132182774
@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 fu
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-132067886
@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 develo
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131881916
@rafaelweingartner it seems fine now, let's await checks and merge
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131881064
Fixed the commit title and message.
Is it appearing as two (2) commits for you?
Here it shows as just one.
---
If your project is set up for
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131877441
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.
---
If your project is se
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131875279
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 gi
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131872061
@rafaelweingartner if you git rebase -i HEAD~2 and have $EDITOR set to
something usable you will get and editor with instructions something like:
```
pic
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131869268
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 pro
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131858619
@rafaelweingartner @cristofolini I love your work, especially the fact the
so many litle unit tests were added. It has some checkstyle errors
unfortunately (in
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/700#discussion_r37198072
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/XenServer56FP1Resource.java
---
@@ -36,18 +31,10 @@
public
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/700#discussion_r37198057
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/XcpServerResource.java
---
@@ -38,20 +32,12 @@
pr
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/700#discussion_r37198033
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/XcpServerResource.java
---
@@ -38,20 +32,12 @@
pr
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131839611
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
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131772545
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131701947
@cristofolini Thanks! As @koushik-das mentions, please squash the commits
and force-push to your PR branch.
@DaanHoogland Currently @wilderrodrigues is o
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131674848
LGTM. Please squash the commits before merge. Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as we
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/700#issuecomment-131468596
@wilderrodrigues this one you should see, thanks for the hard work
@cristofolini.
---
If your project is set up for it, you can reply to this email and have yo
GitHub user cristofolini opened a pull request:
https://github.com/apache/cloudstack/pull/700
Removed duplicate code in CitrixResourceBase and its subclasses
Removed unnecessary duplicated code by having the body of the getPatchFiles
method only in the CitrixResourceBase superclass.
37 matches
Mail list logo
