You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2022/12/08 18:24:59 UTC
[GitHub] [netbeans] lbownik opened a new pull request, #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
lbownik opened a new pull request, #5061:
URL: https://github.com/apache/netbeans/pull/5061
rewritten 2 tests of org.openide.util.TaskTest into more readable form
refactored one
removed tests obsoleted by https://github.com/apache/netbeans/pull/4112
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1064077131
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
@matthiasblaesing @neilcsmith-net are you two also ok with this long-form method naming style for tests? I won't want to be the boo man here and block this for style reasons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
lbownik commented on PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#issuecomment-1372459691
squashed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] ehsavoie commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
ehsavoie commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1055663501
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +303,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
+ throws Exception { // like FolderInstances do
+
class MyTask extends Task {
- private int values;
-
- public MyTask () {
- notifyFinished ();
+
+ private int values = 0;
+
+ public MyTask() {
+ notifyFinished();
}
-
- public void waitFinished () {
- notifyRunning ();
+
+ @Override
+ public void waitFinished() {
+ notifyRunning();
values++;
- notifyFinished ();
+ notifyFinished();
}
}
-
- MyTask my = new MyTask ();
- assertTrue ("The task thinks that he is finished", my.isFinished ());
- assertTrue ("Ok, even with timeout we got the result", my.waitFinished (1000));
- assertEquals ("But the old waitFinished is called", 1, my.values);
+
+ MyTask my = new MyTask();
+ assertTrue(my.isFinished()); //The task thinks that he is finished.
Review Comment:
Please put a message so that the test report is easier to read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1063944744
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
yes but this is simply too long - it doesn't scale. They also do appear in test output windows etc and this makes it hard to read. I would just give up reading after the second compound word and look at the code instead. Esp since some of the names are longer then the test code - its just noise.
`waitOnTask` with a short description if necessary makes it more useful and is also closer to the style already in other tests. Please try to find some middle ground and don't use method name as a full sentence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1064101969
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
squashed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1064018032
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
I shoretened some method names in https://github.com/apache/netbeans/pull/5061/commits/a1d38f89757c486e5c8f9a2e57d28d0a75585068
I'll try to keep a soft 50 character length limit for test method names in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1055684470
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +303,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
+ throws Exception { // like FolderInstances do
+
class MyTask extends Task {
- private int values;
-
- public MyTask () {
- notifyFinished ();
+
+ private int values = 0;
+
+ public MyTask() {
+ notifyFinished();
}
-
- public void waitFinished () {
- notifyRunning ();
+
+ @Override
+ public void waitFinished() {
+ notifyRunning();
values++;
- notifyFinished ();
+ notifyFinished();
}
}
-
- MyTask my = new MyTask ();
- assertTrue ("The task thinks that he is finished", my.isFinished ());
- assertTrue ("Ok, even with timeout we got the result", my.waitFinished (1000));
- assertEquals ("But the old waitFinished is called", 1, my.values);
+
+ MyTask my = new MyTask();
+ assertTrue(my.isFinished()); //The task thinks that he is finished.
Review Comment:
I'm confused - when the test fails just looking at this line makes is obvious what went wrong
Original messages in lines 280-282 were obviously wrong as according to https://junit.org/junit4/javadoc/4.13/org/junit/Assert.html#assertTrue(java.lang.String,%20boolean)
they get displayed only if the assertion fails.
could You propose an example?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1056426037
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +303,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
+ throws Exception { // like FolderInstances do
+
class MyTask extends Task {
- private int values;
-
- public MyTask () {
- notifyFinished ();
+
+ private int values = 0;
+
+ public MyTask() {
+ notifyFinished();
}
-
- public void waitFinished () {
- notifyRunning ();
+
+ @Override
+ public void waitFinished() {
+ notifyRunning();
values++;
- notifyFinished ();
+ notifyFinished();
}
}
-
- MyTask my = new MyTask ();
- assertTrue ("The task thinks that he is finished", my.isFinished ());
- assertTrue ("Ok, even with timeout we got the result", my.waitFinished (1000));
- assertEquals ("But the old waitFinished is called", 1, my.values);
+
+ MyTask my = new MyTask();
+ assertTrue(my.isFinished()); //The task thinks that he is finished.
Review Comment:
fixed in https://github.com/apache/netbeans/pull/5061/commits/4ec2cba5da4d0286cea8daef002e4e095c8e6f00
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1064077131
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
@matthiasblaesing @neilcsmith-net are you two also ok with this long-form method naming style for tests? I won't want to be the boo man here and block this for style reasons or merge something which isn't liked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1064074813
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
ok thanks. looks better. Feel free to squash and force push. Lets get this finally in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien commented on PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#issuecomment-1372986000
restarting the tests now that master is fixed. Hopefully this is getting picked up here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] ehsavoie commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
ehsavoie commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1055690863
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +303,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
+ throws Exception { // like FolderInstances do
+
class MyTask extends Task {
- private int values;
-
- public MyTask () {
- notifyFinished ();
+
+ private int values = 0;
+
+ public MyTask() {
+ notifyFinished();
}
-
- public void waitFinished () {
- notifyRunning ();
+
+ @Override
+ public void waitFinished() {
+ notifyRunning();
values++;
- notifyFinished ();
+ notifyFinished();
}
}
-
- MyTask my = new MyTask ();
- assertTrue ("The task thinks that he is finished", my.isFinished ());
- assertTrue ("Ok, even with timeout we got the result", my.waitFinished (1000));
- assertEquals ("But the old waitFinished is called", 1, my.values);
+
+ MyTask my = new MyTask();
+ assertTrue(my.isFinished()); //The task thinks that he is finished.
Review Comment:
The issue is that when the test fails you have something like : failure line 329 in the test report. Having failure line 329 "The task should have finished" gives more context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1063389250
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
I try to follow the convention "what_behavesHow_underWhatConditions" which I adopted from [Kevlin Henney](https://youtu.be/3frRGQGiPuc?t=2829) - it sorts incely in IDEs class view.
yes - the test methods bacome longer but also more descriptive (though putting some of this description ind annotation might be good).
the reliefing factor is that nobody call these methods directly :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien merged pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien merged PR #5061:
URL: https://github.com/apache/netbeans/pull/5061
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1055757121
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +303,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
+ throws Exception { // like FolderInstances do
+
class MyTask extends Task {
- private int values;
-
- public MyTask () {
- notifyFinished ();
+
+ private int values = 0;
+
+ public MyTask() {
+ notifyFinished();
}
-
- public void waitFinished () {
- notifyRunning ();
+
+ @Override
+ public void waitFinished() {
+ notifyRunning();
values++;
- notifyFinished ();
+ notifyFinished();
}
}
-
- MyTask my = new MyTask ();
- assertTrue ("The task thinks that he is finished", my.isFinished ());
- assertTrue ("Ok, even with timeout we got the result", my.waitFinished (1000));
- assertEquals ("But the old waitFinished is called", 1, my.values);
+
+ MyTask my = new MyTask();
+ assertTrue(my.isFinished()); //The task thinks that he is finished.
Review Comment:
Is there any more generic policy concenring assertion massages I should follow or should I put this message only in this paricular case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1063037518
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +304,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
Review Comment:
the test case methods are getting _really_ long. I personally would prefer having waitOnTask1-x while putting the rest into the javadoc for the method.
I believe junit 5 (which we don't use) has a description annotation. This would also make it easier one day to move the doc comment into an annotation if someone decides to upgrade the testing lib.
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -35,7 +35,8 @@
public class TaskTest {
- private final static long tenMiliseconds = 10000000; // in nanosecods
+ private final static long tenMiliseconds = 10000000; // in nanoseconds
+ private final static long fiveHundredMiliseconds = 500000000; // in nanoseconds
Review Comment:
nitpick for future: this could use [underscores](https://docs.oracle.com/javase/7/docs/technotes/guides/language/underscores-literals.html) to further improve readability. But you added comments which is already great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
mbien commented on PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#issuecomment-1375072589
@matthiasblaesing I agree. I believe @lkishalmi already talked about this [here](https://github.com/apache/netbeans/pull/5115#issuecomment-1363182797). Things like this can be a big time sink for reviewers since the first concern are regressions caused by the refactoring of old tests. The benefit of having nicer looking tests is minimal.
The project has also no lack of tests, its more a lack of resources to run them all at once and also a test reliability problem of often failing tests in some areas.
There are commented out tests here which probably need fixes otherwise they wouldn't be commented out in the first place:
https://github.com/apache/netbeans/blob/master/.github/workflows/main.yml
There are a whole bunch of tests which can't run on JDK 11 which need fixes (but this one is hard since the easy ones are probably already fixed):
https://github.com/apache/netbeans/issues/4904
So there is a lot of test related work available. Its just that refactoring already working tests isn't really solving anything right now.
That being said, I know that you only want to help which is appreciated - believe me. Merging this now. @lbownik thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] matthiasblaesing commented on pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
matthiasblaesing commented on PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#issuecomment-1374928639
The names seem to be ok, but in general I don't think PRs adding tests without fixing a problem are helpful. These bind time for review and discussion, without clear benefit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] desruisseaux commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
desruisseaux commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1055690547
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +303,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
+ throws Exception { // like FolderInstances do
+
class MyTask extends Task {
- private int values;
-
- public MyTask () {
- notifyFinished ();
+
+ private int values = 0;
+
+ public MyTask() {
+ notifyFinished();
}
-
- public void waitFinished () {
- notifyRunning ();
+
+ @Override
+ public void waitFinished() {
+ notifyRunning();
values++;
- notifyFinished ();
+ notifyFinished();
}
}
-
- MyTask my = new MyTask ();
- assertTrue ("The task thinks that he is finished", my.isFinished ());
- assertTrue ("Ok, even with timeout we got the result", my.waitFinished (1000));
- assertEquals ("But the old waitFinished is called", 1, my.values);
+
+ MyTask my = new MyTask();
+ assertTrue(my.isFinished()); //The task thinks that he is finished.
Review Comment:
Opening source files is tedious if the tests were not run in the IDE. So it is useful to have some explanation in the console or test reports. Especially if many tests are failing, a quick look at the error messages help to chose on which test to focus first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] ehsavoie commented on a diff in pull request #5061: rewritten 2 tests of org.openide.util.TaskTest into more readable form
Posted by GitBox <gi...@apache.org>.
ehsavoie commented on code in PR #5061:
URL: https://github.com/apache/netbeans/pull/5061#discussion_r1056122067
##########
platform/openide.util/test/unit/src/org/openide/util/TaskTest.java:
##########
@@ -260,26 +303,32 @@ public void testWaitWithTimeOutReturnsAfterTimeOutWhenTheTaskIsNotComputedAtAll
fail ("Something wrong happened the task should wait for 1000ms but it took: " + time + "\n" + log);
}
+
+ //--------------------------------------------------------------------------
@Test
- public void testWaitOnStrangeTaskThatStartsItsExecutionInOverridenWaitFinishedMethodLikeFolderInstancesDo () throws Exception {
+ public void waitOnTask_thatStartsItsExecutionWithOverridenWaitFinishedMethod()
+ throws Exception { // like FolderInstances do
+
class MyTask extends Task {
- private int values;
-
- public MyTask () {
- notifyFinished ();
+
+ private int values = 0;
+
+ public MyTask() {
+ notifyFinished();
}
-
- public void waitFinished () {
- notifyRunning ();
+
+ @Override
+ public void waitFinished() {
+ notifyRunning();
values++;
- notifyFinished ();
+ notifyFinished();
}
}
-
- MyTask my = new MyTask ();
- assertTrue ("The task thinks that he is finished", my.isFinished ());
- assertTrue ("Ok, even with timeout we got the result", my.waitFinished (1000));
- assertEquals ("But the old waitFinished is called", 1, my.values);
+
+ MyTask my = new MyTask();
+ assertTrue(my.isFinished()); //The task thinks that he is finished.
Review Comment:
More a rule of the thumb: assertEquals usually display both values which usually provides some context while assertNull / notNull/true / false don't provide such context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists