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 2020/08/05 11:25:10 UTC
[GitHub] [commons-io] XenoAmess opened a new pull request #137: add more tests for IOUtils.contentEqualsIgnoreEOL
XenoAmess opened a new pull request #137:
URL: https://github.com/apache/commons-io/pull/137
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] sebbASF commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
sebbASF commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-669157338
What conditions are missed by the existing tests?
In any case, if new tests are needed, they should follow the existing syntax.
Or better, such tests should be independent.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-1082380649
Hi @XenoAmess
Would you rebase on master? I can bring this in then.
--
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] XenoAmess edited a comment on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-683300547
@garydgregory
> When I run `mvn site -P jacoco` on git. master, this method already has 100% code coverage, so more tests are not needed IMO.
Hi.
The new added tests are for pr https://github.com/apache/commons-io/pull/118
Sebb think I should not change test codes in that pr (in order not to break any test, which I agree somehow), so I have to split them out to a new pr.
So this pr SHOULD be reviewed only after https://github.com/apache/commons-io/pull/118
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess edited a comment on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-753646356
> JaCoCo shows this method's coverage is already at 100%/100%, so this does not test anything new. Especially since all EOL handling is handled by the `Reader`'s `readLine()` method, not by our method, so I say let's keep it simple and not test the JRE itself. This PR can just be closed IMO.
coverages can lie, sir :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess edited a comment on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-683300547
@garydgregory
> When I run `mvn site -P jacoco` on git. master, this method already has 100% code coverage, so more tests are not needed IMO.
Hi.
The new added tests are for pr https://github.com/apache/commons-io/pull/118 (for both logical and coverage)
Sebb think I should not change test codes in that pr (in order not to break any test, which I agree somehow), so I have to split them out to a new pr.
So this pr SHOULD be reviewed only after https://github.com/apache/commons-io/pull/118
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-683299705
When I run `mvn site -P jacoco` on git. master, this method already has 100% code coverage, so more tests are not needed IMO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-669194030
@sebbASF
> What conditions are missed by the existing tests?
> In any case, if new tests are needed, they should follow the existing syntax.
> Or better, such tests should be independent.
In short, it lacks situations I added.
In details, it lacks:
1. "" and "\n"
because "" is very tricky, and it is hard to say it for sure whether we think them equal.
2. "a\n" and "a\n\n"
This might sounds stupid, but actually very tricky. because. this function think "a" equals "a\n", but it thinks "a\n" not equals "a\n\n".
Only a non-CRLF char can be follow a ending CRLF and cause no difference to original, in this function.
3. some Readers who same to itself.
The existing tests only have some examples for a same Object, who will be killed exit at the first == check.
But it lacks some real same input test.
4. for every tests, it lacks verse.
whenever check `contentEqualsIgnoreEOF(input1,input2)` be true, must make sure it `contentEqualsIgnoreEOF(input2,input1)` also be true.
5. some \r, \n, \r\n examples.
the original only have one \r\n example, but have no tests for \r and \n.
6. non-equal-length examples.
like "123" and "1234" be false.
they share common prefix, but a longer one is not same.
7. some really not equal examples.
like "1235" and "1234", they really differ on some char, and should return false.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess edited a comment on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-669194030
@sebbASF
> What conditions are missed by the existing tests?
In short, it lacks situations I added.
In details, it lacks:
1. "" and "\n" (also includes "" and "\r", "" and "\r\n"...)
because "" is very tricky, and it is hard to say it for sure whether we think them equal.
2. "a\n" and "a\n\n"
This might sounds stupid, but actually very tricky. because. this function think "a" equals "a\n", but it thinks "a\n" not equals "a\n\n".
Only a non-CRLF char can be follow a ending CRLF and cause no difference to original, in this function.
and another related tricky example is "a\n" and "a\r\n"be equal.
3. some Readers who same to itself.
The existing tests only have some examples for a same Object, who will be killed exit at the first == check.
But it lacks some real same input test.
4. for every tests, it lacks verse.
whenever check `contentEqualsIgnoreEOF(input1,input2)` be true, must make sure it `contentEqualsIgnoreEOF(input2,input1)` also be true.
5. some \r, \n, \r\n examples.
the original only have one \r\n example, but have no tests for \r and \n.
6. non-equal-length examples.
like "123" and "1234" be false.
they share common prefix, but a longer one is not same.
7. some really not equal examples.
like "1235" and "1234", they really differ on some char, and should return false.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-753646356
> JaCoCo shows this method's coverage is already at 100%/100%, so this does not test anything new. Especially since all EOL handling is handled by the `Reader`'s `readLine()` method, not by our method, so I say let's keep it simple and not test the JRE itself. This PR can just be closed IMO.
coverages can lie, sir.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-683300547
@garydgregory
> When I run `mvn site -P jacoco` on git. master, this method already has 100% code coverage, so more tests are not needed IMO.
Hi.
They are for pr https://github.com/apache/commons-io/pull/118
Sebb think I should not change test codes in that pr (in order not to break any test, which I agree somehow), so I have to split them out to a new pr.
So this pr SHOULD be reviewed only after https://github.com/apache/commons-io/pull/118
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess edited a comment on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-669194030
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-753326742
JaCoCo shows this method's coverage is already at 100%/100%, so this does not test anything new. Especially since all EOL handling is handled by the `Reader`'s `readLine()` method, not by our method, so I say let's keep it simple and not test the JRE itself. This PR can just be closed IMO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #137: [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #137:
URL: https://github.com/apache/commons-io/pull/137#issuecomment-1083725670
> Hi @XenoAmess Would you rebase on master? I can bring this in then.
@garydgregory done.
--
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