Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IO-680] Add more tests for IOUtils.contentEqualsIgnoreEOL #137

Conversation

XenoAmess
Copy link
Contributor

No description provided.

@XenoAmess XenoAmess changed the title add more tests for IOUtils.contentEqualsIgnoreEOL [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL Aug 5, 2020
@sebbASF
Copy link
Contributor

sebbASF commented Aug 5, 2020

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.

@XenoAmess XenoAmess force-pushed the add_more_tests_for_IOUtils.contentEqualsIgnoreEOL branch 2 times, most recently from e5242fd to d376b3b Compare August 5, 2020 13:28
@XenoAmess
Copy link
Contributor Author

XenoAmess commented Aug 5, 2020

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

@garydgregory
Copy link
Member

When I run mvn site -P jacoco on git. master, this method already has 100% code coverage, so more tests are not needed IMO.

@XenoAmess
Copy link
Contributor Author

XenoAmess commented Aug 29, 2020

@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 #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 #118

@garydgregory
Copy link
Member

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.

@XenoAmess
Copy link
Contributor Author

XenoAmess commented Jan 3, 2021

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

@garydgregory
Copy link
Member

Hi @XenoAmess
Would you rebase on master? I can bring this in then.

@XenoAmess XenoAmess force-pushed the add_more_tests_for_IOUtils.contentEqualsIgnoreEOL branch from d376b3b to 75951b7 Compare March 30, 2022 22:58
@XenoAmess
Copy link
Contributor Author

Hi @XenoAmess Would you rebase on master? I can bring this in then.

@garydgregory done.

@garydgregory garydgregory merged commit 99e66c0 into apache:master Apr 4, 2022
@garydgregory garydgregory changed the title [IO-680] add more tests for IOUtils.contentEqualsIgnoreEOL [IO-680] Add more tests for IOUtils.contentEqualsIgnoreEOL Apr 4, 2022
DaGeRe pushed a commit to DaGeRe/commons-io that referenced this pull request Apr 5, 2022
DaGeRe pushed a commit to DaGeRe/commons-io that referenced this pull request Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants