[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-211359351
  
@swill thanks, I was expecting for this one a long time



---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1445


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209024302
  
I did nto noticed that @swill  LGTM.
So, it is OK.


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[rafaelweingartner]

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1445#discussion_r59422694
  
--- Diff: utils/src/test/java/com/cloud/utils/TestProfiler.java ---
@@ -19,84 +19,79 @@
 
 package com.cloud.utils;
 
-import org.apache.log4j.Logger;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
 import com.cloud.utils.testcase.Log4jEnabledTestCase;
 
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({Profiler.class})
 public class TestProfiler extends Log4jEnabledTestCase {
-protected final static Logger s_logger = 
Logger.getLogger(TestProfiler.class);
 
-private static final long MILLIS_FACTOR = 1000l;
-private static final int MARGIN = 100;
-// Profiler uses System.nanoTime which is not reliable on the 
millisecond
-// A sleep of 1000 milliseconds CAN be measured as 999 milliseconds
-// Therefore make the second larger, by 10% of the margin
-private static final long ONE_SECOND = 1000l + (MARGIN / 10);
-private static final double EXPONENT = 3d;
+private static final long SLEEP_TIME_NANO = 10L;
+private static Profiler pf;
+
+@Before
+public void setUp() {
+pf = new Profiler();
+PowerMockito.mockStatic(System.class);
+PowerMockito.when(System.nanoTime()).thenReturn(0L, 
SLEEP_TIME_NANO);
+}
 
 @Test
 public void testProfilerInMillis() {
-s_logger.info("testProfiler() started");
+//Given
--- End diff --

@alexandrelimassantana  these comments "//Given", "//When", and so forth 
could be removed


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209028959
  
I appreciate it.  :+1: 


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209025189
  
@pdube sorry, my brain is into something else right now and I didn't review 
the scope of the code.  Yes, your tests are valid tests for this change.  So 
yes, I will add this to my queue to be merged.  Thx guys...


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[pdube]

Github user pdube commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209026629
  
Just trying to help get things merged :)


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[pdube]

Github user pdube commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209023846
  
@rafaelweingartner @swill The only thing changed was the unit tests, which 
I ran. And the only code change he did was to remove useless variables. @swill 
also gave his LGTM higher


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209021841
  
I would say it is missing an LGTM.
But, it is very good the code.


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209020807
  
@pdube we need to run CI against it first, but otherwise, yes it is ready...


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[pdube]

Github user pdube commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-209017286
  
@swill this looks ready, no?


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[pdube]

Github user pdube commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-199858341
  
Built and ran the unit tests. LGTM

[INFO] Apache CloudStack Developer Tools - Checkstyle Configuration  
SUCCESS [  1.106 s]
[INFO] Apache CloudStack . SUCCESS [  0.975 
s]
[INFO] Apache CloudStack Maven Conventions Parent  SUCCESS [  1.918 
s]
[INFO] Apache CloudStack Framework - Managed Context . SUCCESS [  1.485 
s]
[INFO] Apache CloudStack Utils ... SUCCESS [ 11.555 
s]
[INFO] Apache CloudStack Framework ... SUCCESS [  0.047 
s]
[INFO] Apache CloudStack Framework - Event Notification .. SUCCESS [  1.561 
s]
[INFO] Apache CloudStack Framework - Configuration ... SUCCESS [  1.210 
s]
[INFO] Apache CloudStack API . SUCCESS [ 12.870 
s]
[INFO] Apache CloudStack Framework - REST  SUCCESS [  0.644 
s]
[INFO] Apache CloudStack Framework - IPC . SUCCESS [  0.640 
s]
[INFO] Apache CloudStack Cloud Engine  SUCCESS [  0.030 
s]
[INFO] Apache CloudStack Cloud Engine API  SUCCESS [  3.529 
s]
[INFO] Apache CloudStack Framework - Security  SUCCESS [  0.499 
s]
[INFO] Apache CloudStack Core  SUCCESS [  4.886 
s]
[INFO] Apache CloudStack Agents .. SUCCESS [  1.546 
s]
[INFO] Apache CloudStack Framework - Clustering .. SUCCESS [  1.026 
s]
[INFO] Apache CloudStack Framework - Event Notification .. SUCCESS [  0.332 
s]
[INFO] Apache CloudStack Cloud Engine Schema Component ... SUCCESS [ 10.903 
s]
[INFO] Apache CloudStack Framework - Jobs  SUCCESS [  2.049 
s]
[INFO] Apache CloudStack Cloud Engine Internal Components API  SUCCESS [  
0.551 s]
[INFO] Apache CloudStack Server .. SUCCESS [ 25.862 
s]
[INFO] Apache CloudStack Framework - Quota ... SUCCESS [  1.987 
s]
[INFO] Apache CloudStack Usage Server  SUCCESS [  1.694 
s]
[INFO] Apache CloudStack Cloud Engine Orchestration Component  SUCCESS [  
0.648 s]
[INFO] Apache CloudStack Cloud Services .. SUCCESS [  0.016 
s]
[INFO] Apache CloudStack Secondary Storage ... SUCCESS [  0.125 
s]
[INFO] Apache CloudStack Secondary Storage Service ... SUCCESS [  0.678 
s]
[INFO] Apache CloudStack Engine Storage Component  SUCCESS [  5.129 
s]
[INFO] Apache CloudStack Engine Storage Volume Component . SUCCESS [  0.588 
s]
[INFO] Apache CloudStack Engine Storage Image Component .. SUCCESS [  0.472 
s]
[INFO] Apache CloudStack Engine Storage Data Motion Component  SUCCESS [  
0.592 s]
[INFO] Apache CloudStack Engine Storage Cache Component .. SUCCESS [  0.545 
s]
[INFO] Apache CloudStack Engine Storage Snapshot Component  SUCCESS [  
3.074 s]
[INFO] Apache CloudStack Cloud Engine API  SUCCESS [  0.567 
s]
[INFO] Apache CloudStack Cloud Engine Service  SUCCESS [  0.596 
s]
[INFO] Apache CloudStack Plugin POM .. SUCCESS [  0.266 
s]
[INFO] Apache CloudStack Plugin - API Rate Limit . SUCCESS [  3.276 
s]
[INFO] Apache CloudStack Plugin - Storage Volume default provider  SUCCESS 
[  0.485 s]
[INFO] Apache CloudStack Plugin - Storage Volume SolidFire Provider  
SUCCESS [  0.574 s]
[INFO] Apache CloudStack Plugin - API SolidFire .. SUCCESS [  0.483 
s]
[INFO] Apache CloudStack Plugin - API Discovery .. SUCCESS [  1.377 
s]
[INFO] Apache CloudStack Plugin - ACL Static Role Based .. SUCCESS [  0.497 
s]
[INFO] Apache CloudStack Plugin - Host Anti-Affinity Processor  SUCCESS [  
0.428 s]
[INFO] Apache CloudStack Plugin - Explicit Dedication Processor  SUCCESS [  
0.404 s]
[INFO] Apache CloudStack Plugin - User Concentrated Pod Deployment Planner  
SUCCESS [  0.429 s]
[INFO] Apache CloudStack Plugin - User Dispersing Deployment Planner  
SUCCESS [  0.424 s]
[INFO] Apache CloudStack Plugin - Implicit Dedication Planner  SUCCESS [  
2.820 s]
[INFO] Apache CloudStack Plugin - Skip Heurestics Planner  SUCCESS [  0.635 
s]
[INFO] Apache CloudStack Plugin - Host Allocator Random .. SUCCESS [  0.483 
s]
[INFO] Apache CloudStack Plugin - Dedicated Resources  SUCCESS [  2.748 
s]
[INFO] Apache CloudStack Plugin - Hypervisor OracleVM  SUCCESS [  0.560 
s]
[INFO] Apache CloudStack Plugin - Open vSwitch ... SUCCESS [  0.566 
s]
[INFO] Apache CloudStack Plugin - Hypervisor XenServer ... SUCCESS [ 21.803 
s]
[INFO] Apache CloudStack Plugin - Hypervisor KVM . SUCCESS [  3.487 
s]
[INFO] Apache CloudStack Plugin - RabbitMQ Event Bus . SUCCESS [  0.526 
s]
[INFO] Apache CloudStack Plugin - 

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1445#issuecomment-199535142
  
This LGTM.  @pdube, would you mind reviewing this for me?


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1445#discussion_r56813778
  
--- Diff: utils/src/test/java/com/cloud/utils/TestProfiler.java ---
@@ -19,84 +19,79 @@
 
 package com.cloud.utils;
 
-import org.apache.log4j.Logger;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
 import com.cloud.utils.testcase.Log4jEnabledTestCase;
 
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({Profiler.class})
 public class TestProfiler extends Log4jEnabledTestCase {
-protected final static Logger s_logger = 
Logger.getLogger(TestProfiler.class);
 
-private static final long MILLIS_FACTOR = 1000l;
-private static final int MARGIN = 100;
-// Profiler uses System.nanoTime which is not reliable on the 
millisecond
-// A sleep of 1000 milliseconds CAN be measured as 999 milliseconds
-// Therefore make the second larger, by 10% of the margin
-private static final long ONE_SECOND = 1000l + (MARGIN / 10);
-private static final double EXPONENT = 3d;
+private static final long SLEEP_TIME_NANO = 10L;
+private static Profiler pf;
+
+@Before
+public void setUp() {
+pf = new Profiler();
+PowerMockito.mockStatic(System.class);
+PowerMockito.when(System.nanoTime()).thenReturn(0L, 
SLEEP_TIME_NANO);
--- End diff --

@alexandrelimassantana , You are absolutely right, i've been reading 
without glasses. nice work.


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[alexandrelimassantana]

Github user alexandrelimassantana commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1445#discussion_r56808020
  
--- Diff: utils/src/test/java/com/cloud/utils/TestProfiler.java ---
@@ -19,84 +19,79 @@
 
 package com.cloud.utils;
 
-import org.apache.log4j.Logger;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
 import com.cloud.utils.testcase.Log4jEnabledTestCase;
 
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({Profiler.class})
 public class TestProfiler extends Log4jEnabledTestCase {
-protected final static Logger s_logger = 
Logger.getLogger(TestProfiler.class);
 
-private static final long MILLIS_FACTOR = 1000l;
-private static final int MARGIN = 100;
-// Profiler uses System.nanoTime which is not reliable on the 
millisecond
-// A sleep of 1000 milliseconds CAN be measured as 999 milliseconds
-// Therefore make the second larger, by 10% of the margin
-private static final long ONE_SECOND = 1000l + (MARGIN / 10);
-private static final double EXPONENT = 3d;
+private static final long SLEEP_TIME_NANO = 10L;
+private static Profiler pf;
+
+@Before
+public void setUp() {
+pf = new Profiler();
+PowerMockito.mockStatic(System.class);
+PowerMockito.when(System.nanoTime()).thenReturn(0L, 
SLEEP_TIME_NANO);
--- End diff --

The statement on line 43 is saying that on the first call to nanoTime it 
will return 0 and the second call will return 10^9 ( 1 second in nanoseconds ).

when start() calls nanoTime() first, it gets 0 and when stop() calls it 
again, the result will be 10^9. That is how it simulates the passage of 1s 
without actually making the thread sleep for 1 second.

The tests previously written about this class are not intended to test the 
capability of the System class to capture time nor the Thread.sleep() method ( 
which was causing the test to fail as I stated because of the low priority the 
JVM's proccess have on different OS ). As it is right now, it stills test the 
same functionalities... Just without the sleep and System direct calls.


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1445#discussion_r56799125
  
--- Diff: utils/src/test/java/com/cloud/utils/TestProfiler.java ---
@@ -19,84 +19,79 @@
 
 package com.cloud.utils;
 
-import org.apache.log4j.Logger;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
 import com.cloud.utils.testcase.Log4jEnabledTestCase;
 
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({Profiler.class})
 public class TestProfiler extends Log4jEnabledTestCase {
-protected final static Logger s_logger = 
Logger.getLogger(TestProfiler.class);
 
-private static final long MILLIS_FACTOR = 1000l;
-private static final int MARGIN = 100;
-// Profiler uses System.nanoTime which is not reliable on the 
millisecond
-// A sleep of 1000 milliseconds CAN be measured as 999 milliseconds
-// Therefore make the second larger, by 10% of the margin
-private static final long ONE_SECOND = 1000l + (MARGIN / 10);
-private static final double EXPONENT = 3d;
+private static final long SLEEP_TIME_NANO = 10L;
+private static Profiler pf;
+
+@Before
+public void setUp() {
+pf = new Profiler();
+PowerMockito.mockStatic(System.class);
+PowerMockito.when(System.nanoTime()).thenReturn(0L, 
SLEEP_TIME_NANO);
--- End diff --

mocking the system return of the time does not seem right to me. it will be 
returning the same in start() and in stop(), don't you agree?


---
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.
---

[GitHub] cloudstack pull request: Fixed Profiler's unit tests bugs.

[alexandrelimassantana]

GitHub user alexandrelimassantana opened a pull request:

https://github.com/apache/cloudstack/pull/1445

Fixed Profiler's unit tests bugs.

Problem:
The TestProfiler class was using Java Thread methods to test the
Profiler's functionality. That was causing the tests to fail sometimes
since the JVM's thread priority could be low on some OS. 

Fix:
Using PowerMockito to mock the System calls, the threads could be
removed. This makes the tests considerably faster, OS independent and
still guarantees the correct implementation of the Profiler class.

The changes on the Profiler's class was only to shorten the class's line
size by not assigning the return value to a variable returning it
straight out.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/rafaelweingartner/cloudstack 
lrg-cs-hackday-025

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1445.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1445


commit 8d6370b21fd527dc0046c0a1d67dce35dbf15681
Author: Alexandre Limas Santana 
Date:   2016-03-19T19:25:47Z

Fixed Profiler's unit tests bugs.

Problem:
The TestProfiler class was using Java Thread methods to test the
Profiler's functionality. That was causing the tests to fail sometimes
since the JVM's thread priority could be low on some OS. 

Fix:
Using PowerMockito to mock the System calls, the threads could be
removed. This makes the tests considerably faster, OS independent and
still guarantees the correct implementation of the Profiler class.

The changes on the Profiler's class was only to shorten the class's line
size by not assigning the return value to a variable returning it
straight out.




---
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.
---