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/03/30 03:57:35 UTC

[GitHub] [netbeans] lbownik opened a new pull request #3894: Lbownik dev

lbownik opened a new pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894


   improved tests for BaseUtilities.toURI  & BaseUtitlites.toFile
   
   
   ---
   **^Add meaningful description above**
   
   By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
   
    - are all your own work, and you have the right to contribute them.
    - are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).
   
   Please make sure (eg. `git log`) that all commits have a valid name and email address for you in the Author field.
   
   If you're a first time contributor, see the Contributing guidelines for more information.
   
   


-- 
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 change in pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
mbien commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r839012669



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -126,8 +206,10 @@ public void testFileURI() throws Exception {
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:///some/path")));
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:/some/path/")));
         }
+        //the following code seems not to have sense at all

Review comment:
       i think it wants to test the method `BaseUtilities.toURI()` against expected results taken by using URI directly. It doesn't look too bad in my eyes.




-- 
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 closed pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
lbownik closed pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894


   


-- 
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] vieiro commented on a change in pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
vieiro commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r838847238



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -111,7 +112,86 @@ public void testIsJavaIdentifier() throws Exception {
         assertFalse(BaseUtilities.isJavaIdentifier("some.thing"));
     }
 
+    //--------------------------------------------------------------------------
+    public void test_toFile_throwsNullPonter_whenArgumentIsNull()
+            throws Exception {
+
+        try {
+            toFile(null);
+            fail();
+        } catch (final NullPointerException e) {
+            return;
+        }
+    }
+
+    //--------------------------------------------------------------------------
+    public void test_toFile_returnsFile_whenGivenUnixPath()

Review comment:
       I see! Thanks for the explanation. It's good to have these comments in the PR, so future reviewers can understand more easily!




-- 
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 change in pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
mbien commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r838998547



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -38,65 +39,65 @@ public BaseUtilitiesTest(String name) {
     @Override
     protected void setUp() throws Exception {
         super.setUp();
-        BaseUtilities.resetOperatingSystem ();
+        BaseUtilities.resetOperatingSystem();
         originalOsName = System.getProperty("os.name");
     }
-    
+
     @Override
     protected void tearDown() throws Exception {
         System.setProperty("os.name", originalOsName);
         super.tearDown();
     }
 
-    public void testGetOperatingSystemWinNT () {
-        System.setProperty ("os.name", "Windows NT");
+    public void testGetOperatingSystemWinNT() {
+        System.setProperty("os.name", "Windows NT");
         //assertEquals ("System.getProperty (os.name) returns Windows NT", "Windows NT", System.getProperty ("os.name"));
-        assertEquals ("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem ());
+        assertEquals("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem());

Review comment:
       there is a little trick you can do if you really really want to sneak reformats into a PR which is not related: you keep them in a separate commit and mention it in the PR text (but don't overdo that). So reviewers can concentrate on the non-stylistic changes, but still glance over the formatting commit and look at it separately from the actual issue. Reducing noise is always helpful. (it also makes it much easier to revert if the reviewers don't want the formatting changes)




-- 
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 change in pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
mbien commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r838998547



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -38,65 +39,65 @@ public BaseUtilitiesTest(String name) {
     @Override
     protected void setUp() throws Exception {
         super.setUp();
-        BaseUtilities.resetOperatingSystem ();
+        BaseUtilities.resetOperatingSystem();
         originalOsName = System.getProperty("os.name");
     }
-    
+
     @Override
     protected void tearDown() throws Exception {
         System.setProperty("os.name", originalOsName);
         super.tearDown();
     }
 
-    public void testGetOperatingSystemWinNT () {
-        System.setProperty ("os.name", "Windows NT");
+    public void testGetOperatingSystemWinNT() {
+        System.setProperty("os.name", "Windows NT");
         //assertEquals ("System.getProperty (os.name) returns Windows NT", "Windows NT", System.getProperty ("os.name"));
-        assertEquals ("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem ());
+        assertEquals("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem());

Review comment:
       there is a little trick you can do if you really really want to sneak reformats into a PR which is not related: you keep them in a separate commit and mention it in the PR text (but don't overdo that). So reviewers can concentrate on the non-stylistic changes, but still glance over the formatting commit and look at it separately from the actual issue. Reducing noise is always helpful.




-- 
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] vieiro commented on a change in pull request #3894: Lbownik dev

Posted by GitBox <gi...@apache.org>.
vieiro commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r838534419



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -38,65 +39,65 @@ public BaseUtilitiesTest(String name) {
     @Override
     protected void setUp() throws Exception {
         super.setUp();
-        BaseUtilities.resetOperatingSystem ();
+        BaseUtilities.resetOperatingSystem();
         originalOsName = System.getProperty("os.name");
     }
-    
+
     @Override
     protected void tearDown() throws Exception {
         System.setProperty("os.name", originalOsName);
         super.tearDown();
     }
 
-    public void testGetOperatingSystemWinNT () {
-        System.setProperty ("os.name", "Windows NT");
+    public void testGetOperatingSystemWinNT() {
+        System.setProperty("os.name", "Windows NT");
         //assertEquals ("System.getProperty (os.name) returns Windows NT", "Windows NT", System.getProperty ("os.name"));
-        assertEquals ("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem ());
+        assertEquals("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem());

Review comment:
       Reformatting existing code makes reviews harder. We don't want to reformat existing code without a real need. Otherwise we'll be revieweing reformats for each one of the NetBeans committers.

##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -126,8 +206,10 @@ public void testFileURI() throws Exception {
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:///some/path")));
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:/some/path/")));
         }
+        //the following code seems not to have sense at all

Review comment:
       Why?

##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -111,7 +112,86 @@ public void testIsJavaIdentifier() throws Exception {
         assertFalse(BaseUtilities.isJavaIdentifier("some.thing"));
     }
 
+    //--------------------------------------------------------------------------
+    public void test_toFile_throwsNullPonter_whenArgumentIsNull()
+            throws Exception {
+
+        try {
+            toFile(null);
+            fail();
+        } catch (final NullPointerException e) {
+            return;
+        }
+    }
+
+    //--------------------------------------------------------------------------
+    public void test_toFile_returnsFile_whenGivenUnixPath()

Review comment:
       Is this test already included in some of the other tests? Why is is neccessary?




-- 
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] vieiro commented on a change in pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
vieiro commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r838847765



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -126,8 +206,10 @@ public void testFileURI() throws Exception {
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:///some/path")));
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:/some/path/")));
         }
+        //the following code seems not to have sense at all

Review comment:
       Again. this comment in the PR, so reviewers undestand what you're trying to do. 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] vieiro commented on pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
vieiro commented on pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#issuecomment-1083476703


   Hi @lbownik ,
   No worries for the PR title and the merge with master (as you see the merge with master commit has added no changes).
   The tests fail now in the "Travis CI" on Linux AMD64 JDK8:
   
   ```
   OPTS="-Dmetabuild.jsonurl=https://raw.githubusercontent.com/apache/netbeans-jenkins-lib/master/meta/netbeansrelease.json -silent -Dcluster.config=platform -Djavac.compilerargs=-nowarn -Dbuild.compiler.deprecation=false -Dtest-unit-sys-prop.ignore.random.failures=true -Dvanilla.javac.exists=true" 
   ```
   
   ```
   nbbuild/travis/print-junit-report.sh
   =================== JUnit Report Summary / failed tests ===================
   org.openide.util.BaseUtilitiesTest
         failed: test_toURI_returnsURI_whenGivenCorrectUnixPath
   ====================== JUnit failure details ===============================
   Suite: org.openide.util.BaseUtilitiesTest
         test_toURI_returnsURI_whenGivenCorrectUnixPath FAILED :  expected:&lt;file:/some/path/&gt; but was:&lt;file:/some/path&gt;
             junit.framework.AssertionFailedError: expected:&lt;file:/some/path/&gt; but was:&lt;file:/some/path&gt;
             	at org.openide.util.BaseUtilitiesTest.test_toURI_returnsURI_whenGivenCorrectUnixPath(BaseUtilitiesTest.java:171)
             	at org.netbeans.junit.NbTestCase.access$200(NbTestCase.java:77)
             	at org.netbeans.junit.NbTestCase$2.doSomething(NbTestCase.java:476)
             	at org.netbeans.junit.NbTestCase$1Guard.run(NbTestCase.java:402)
             	at java.lang.Thread.run(Thread.java:748)
   ------------- End suite org.openide.util.BaseUtilitiesTest ------------
   ======================= End of JUnit report ===============================
   ```


-- 
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 change in pull request #3894: Lbownik dev

Posted by GitBox <gi...@apache.org>.
lbownik commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r838765335



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -111,7 +112,86 @@ public void testIsJavaIdentifier() throws Exception {
         assertFalse(BaseUtilities.isJavaIdentifier("some.thing"));
     }
 
+    //--------------------------------------------------------------------------
+    public void test_toFile_throwsNullPonter_whenArgumentIsNull()
+            throws Exception {
+
+        try {
+            toFile(null);
+            fail();
+        } catch (final NullPointerException e) {
+            return;
+        }
+    }
+
+    //--------------------------------------------------------------------------
+    public void test_toFile_returnsFile_whenGivenUnixPath()

Review comment:
       I started with this because I wanted to shave off 2 seconds of execution from the "testFileURI" test (I wanted to start with something simple just to warm up). Then I realized that "testFileURI" is kind of messy , so I started to dissecting it into smaller pieces. The new tets methods are intended to replase the "testFileURI" test. I intend to remove "testFileURI" in next PR after succesful nightly build (so that there is proof that both old and new test pass).

##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -126,8 +206,10 @@ public void testFileURI() throws Exception {
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:///some/path")));
             assertEquals(new File("/some/path"), BaseUtilities.toFile(new URI("file:/some/path/")));
         }
+        //the following code seems not to have sense at all

Review comment:
       Because it tests URI class and not toURI method - even though it call it. 
   It seems to be a test of JVM implementation itself - I have no idea what for.




-- 
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 change in pull request #3894: Improved tests for BaseUtilities.toURI & BaseUtitlites.toFile

Posted by GitBox <gi...@apache.org>.
mbien commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r839004972



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -111,7 +112,86 @@ public void testIsJavaIdentifier() throws Exception {
         assertFalse(BaseUtilities.isJavaIdentifier("some.thing"));
     }
 
+    //--------------------------------------------------------------------------
+    public void test_toFile_throwsNullPonter_whenArgumentIsNull()
+            throws Exception {
+
+        try {
+            toFile(null);
+            fail();
+        } catch (final NullPointerException e) {
+            return;
+        }
+    }
+
+    //--------------------------------------------------------------------------
+    public void test_toFile_returnsFile_whenGivenUnixPath()

Review comment:
       > The new tets methods are intended to replase the "testFileURI" test. I intend to remove "testFileURI" in next PR after succesful nightly build (so that there is proof that both old and new test pass).
   
   good thinking but there is no need to wait. PRs should be somewhat standalone if possible. Look at it this way:
    - we know that the old tests are green before this PR
    - this PR is already built and tested (I just unlocked the gh workflow here for another set of tests) and will be automatically tested again post merge (-> there already is a documented before and after)
    - so feel free to remove the old test case which is supposed to be replaced in the same PR you add the replacement cases




-- 
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 change in pull request #3894: Lbownik dev

Posted by GitBox <gi...@apache.org>.
lbownik commented on a change in pull request #3894:
URL: https://github.com/apache/netbeans/pull/3894#discussion_r838759941



##########
File path: platform/openide.util/test/unit/src/org/openide/util/BaseUtilitiesTest.java
##########
@@ -38,65 +39,65 @@ public BaseUtilitiesTest(String name) {
     @Override
     protected void setUp() throws Exception {
         super.setUp();
-        BaseUtilities.resetOperatingSystem ();
+        BaseUtilities.resetOperatingSystem();
         originalOsName = System.getProperty("os.name");
     }
-    
+
     @Override
     protected void tearDown() throws Exception {
         System.setProperty("os.name", originalOsName);
         super.tearDown();
     }
 
-    public void testGetOperatingSystemWinNT () {
-        System.setProperty ("os.name", "Windows NT");
+    public void testGetOperatingSystemWinNT() {
+        System.setProperty("os.name", "Windows NT");
         //assertEquals ("System.getProperty (os.name) returns Windows NT", "Windows NT", System.getProperty ("os.name"));
-        assertEquals ("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem ());
+        assertEquals("Windows NT recognized as OS_WINNT", BaseUtilities.OS_WINNT, BaseUtilities.getOperatingSystem());

Review comment:
       Proper formating is important but I get the idea. Next Time if I reallt, really need to format something I will provide PR just with formating change. 




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