You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/05/19 12:22:59 UTC

[GitHub] [commons-io] lacinoire opened a new pull request, #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

lacinoire opened a new pull request, #358:
URL: https://github.com/apache/commons-io/pull/358

   Hey 😊
   I want to contribute the following test:
   
   Test that `baout.toString()` is equal to `""` when `reset` is called. 
   This tests the methods [`AbstractByteArrayOutputStream.toByteArrayImpl`](https://github.com/apache/commons-io/blob/4aab769c3279ea63b7bcdaa163c50761bc540f8c/src/main/java/org/apache/commons/io/output/AbstractByteArrayOutputStream.java#L188) and [`AbstractByteArrayOutputStream.resetImpl`](https://github.com/apache/commons-io/blob/4aab769c3279ea63b7bcdaa163c50761bc540f8c/src/main/java/org/apache/commons/io/output/AbstractByteArrayOutputStream.java#L148). 
   This test is based on the test [`copy_inputStreamToWriter`](https://github.com/apache/commons-io/blob/4aab769c3279ea63b7bcdaa163c50761bc540f8c/src/test/java/org/apache/commons/io/CopyUtilsTest.java#L83).
   
   Curious to hear what you think!
   
   (I wrote this test as part of a research study at TU Delft. [Find out more](https://github.com/lacinoire/lacinoire/blob/main/README.md))


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] lacinoire commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

lacinoire commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1150753944

   > That's not enough, you want to run "mvn" which will run the default goal which runs more checks.
   
   Also passes ✅ 
   
   Curious if the coverage.yml run shows any 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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on a diff in pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

garydgregory commented on code in PR #358:
URL: https://github.com/apache/commons-io/pull/358#discussion_r878919402


##########
src/test/java/org/apache/commons/io/output/ByteArrayOutputStreamTest.java:
##########
@@ -350,6 +360,22 @@ public void testWriteZero(final String baosName, final BAOSFactory<?> baosFactor
         }
     }
 
+    private static final int FILE_SIZE = (1024 * 4) + 1;
+    private final byte[] inData = TestUtils.generateTestData(FILE_SIZE);
+
+    @Test
+    public void testToByteArrayImplAndResetImpl() throws Exception {
+        InputStream in = new ByteArrayInputStream(inData);

Review Comment:
   Use try with resources when possible.
   



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] lacinoire commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

lacinoire commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1149917758

   Thank you for the second round of comments 🙂
   I addressed them and also made sure that `mvn checkstyle:check`  and `mvn verify clean` pass.
   Hope the CI does to 😄


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] codecov-commenter commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

codecov-commenter commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1151017018

   # [Codecov](https://codecov.io/gh/apache/commons-io/pull/358?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#358](https://codecov.io/gh/apache/commons-io/pull/358?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8cc6b76) into [master](https://codecov.io/gh/apache/commons-io/commit/5dfe7fa4e28cab0b2003d4bc3a3b430b0da78c44?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5dfe7fa) will **decrease** coverage by `0.02%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #358      +/-   ##
   ============================================
   - Coverage     84.47%   84.44%   -0.03%     
     Complexity     2983     2983              
   ============================================
     Files           192      192              
     Lines          7336     7336              
     Branches        974      974              
   ============================================
   - Hits           6197     6195       -2     
   - Misses          889      891       +2     
     Partials        250      250              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/commons-io/pull/358?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../main/java/org/apache/commons/io/input/Tailer.java](https://codecov.io/gh/apache/commons-io/pull/358/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vaW5wdXQvVGFpbGVyLmphdmE=) | `86.06% <0.00%> (-1.00%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/commons-io/pull/358?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/commons-io/pull/358?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5dfe7fa...8cc6b76](https://codecov.io/gh/apache/commons-io/pull/358?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

garydgregory commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1151118168

   Hi @lacinoire
   
   I'm not sure we need this test unless you can explain what it tests that the exising infrastructure does not in:
   - `org.apache.commons.io.output.ByteArrayOutputStreamTest.testStream(String, BAOSFactory)`
   - `org.apache.commons.io.output.ByteArrayOutputStreamTest.testToInputStreamWithReset(String, BAOSFactory)`
   
   FWIW, I cannot explain why the test coverage for this PR is different/worse.
   


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

garydgregory commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1150123687

   That's not enough, you want to run "mvn" which will run the default goal which runs more checks.


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on a diff in pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

garydgregory commented on code in PR #358:
URL: https://github.com/apache/commons-io/pull/358#discussion_r878919402


##########
src/test/java/org/apache/commons/io/output/ByteArrayOutputStreamTest.java:
##########
@@ -350,6 +360,22 @@ public void testWriteZero(final String baosName, final BAOSFactory<?> baosFactor
         }
     }
 
+    private static final int FILE_SIZE = (1024 * 4) + 1;
+    private final byte[] inData = TestUtils.generateTestData(FILE_SIZE);
+
+    @Test
+    public void testToByteArrayImplAndResetImpl() throws Exception {
+        InputStream in = new ByteArrayInputStream(inData);

Review Comment:
   Use try with resources
   



##########
src/test/java/org/apache/commons/io/output/ByteArrayOutputStreamTest.java:
##########
@@ -350,6 +360,22 @@ public void testWriteZero(final String baosName, final BAOSFactory<?> baosFactor
         }
     }
 
+    private static final int FILE_SIZE = (1024 * 4) + 1;
+    private final byte[] inData = TestUtils.generateTestData(FILE_SIZE);

Review Comment:
   These data do not need to be tracked for all tests. Move them inside the one method that use them.



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] lacinoire closed pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

lacinoire closed pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl
URL: https://github.com/apache/commons-io/pull/358


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on a diff in pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

garydgregory commented on code in PR #358:
URL: https://github.com/apache/commons-io/pull/358#discussion_r889363755


##########
src/test/java/org/apache/commons/io/output/ByteArrayOutputStreamTest.java:
##########
@@ -350,6 +358,22 @@ public void testWriteZero(final String baosName, final BAOSFactory<?> baosFactor
         }
     }
 
+    @Test
+    public void testToByteArrayImplAndResetImpl() throws Exception {
+        final int file_size = (1024 * 4) + 1;
+        final byte[] inData = TestUtils.generateTestData(file_size);
+        try (InputStream in = new ByteArrayInputStream(inData)) {
+            try (ByteArrayOutputStream baout = new ByteArrayOutputStream()) {
+                try (Writer writer = new OutputStreamWriter(baout, StandardCharsets.US_ASCII)) {
+                    CopyUtils.copy(in, writer);
+                    writer.flush();
+                }
+                baout.reset();
+                Assertions.assertEquals("", baout.toString());

Review Comment:
   No need for Assertions prefix static in a test method. Use the other methods in this file as a style guide.



##########
src/test/java/org/apache/commons/io/output/ByteArrayOutputStreamTest.java:
##########
@@ -350,6 +358,22 @@ public void testWriteZero(final String baosName, final BAOSFactory<?> baosFactor
         }
     }
 
+    @Test
+    public void testToByteArrayImplAndResetImpl() throws Exception {
+        final int file_size = (1024 * 4) + 1;

Review Comment:
   - No underscore in local variable names. 
   - No need for spelling out 4097 in an expression.



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

garydgregory commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1150146756

   Hi @lacinoire,
   Would you please rebase on master? This will pick up adding running coverage.yml.
   TY!


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] lacinoire commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

lacinoire commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1145995945

   Thank you for the review!
   I incorporated your suggestions 🙂


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [commons-io] lacinoire commented on pull request #358: Add test for AbstractByteArrayOutputStream.resetImpl and .toByteArrayImpl

Posted by GitBox <gi...@apache.org>.

lacinoire commented on PR #358:
URL: https://github.com/apache/commons-io/pull/358#issuecomment-1153842180

   Hey @garydgregory,
   looked into those two tests in detail and indeed, as far as I can see they should also cover most the instructions I was targeting. Also they should covere whole new branches which i clearly not the case, trusting covdecov here.
   I will go back and check why our own coverage calculation is so off in this case.
   
   Thank you for the in-depth feedback and help with this PR! :pray: 🙂 


-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org