Skip to content

add-some-Unit-Tests Added some Unit Tests to increase code coverage. - #33

Closed
TheRealHaui wants to merge 6 commits into
apache:masterfrom
TheRealHaui:add-some-Unit-Tests
Closed

add-some-Unit-Tests Added some Unit Tests to increase code coverage.#33
TheRealHaui wants to merge 6 commits into
apache:masterfrom
TheRealHaui:add-some-Unit-Tests

Conversation

@TheRealHaui

Copy link
Copy Markdown
Contributor

I have added some Unit Tests to increase code coverage and want to contribute them.

@TheRealHaui

Copy link
Copy Markdown
ContributorAuthor

@bodewig
Could you be so kind an review my contribution and - of course - most of all pull it into the code base.

@coveralls

coveralls commented Jun 13, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.2%) to 84.524% when pulling 2f45456 on TheRealHaui:add-some-Unit-Tests into 0d8ab18 on apache:master.

@bodewig

Copy link
Copy Markdown
Member

Fortunately I'm not the only one who could merge this :-)

More seriously, it may take a bit of time until I get there, but I will.

@bodewig

Copy link
Copy Markdown
Member

Thanks @TheRealHaui

I agree with your comment on ChecksumCalculatingInputStreamTest#testGetValueThrowsNullPointerException this looks like a bug. So I'd rather remove the test and see the bug fixed. Do you want to take a stab?

Also, the Commons Compress community has decided to not use author tags, I'd have to strip yours before merging the PR. Could you please remove them yourself if you modify this PR? Feel free to add yourself as a contributor to the POM.

@coveralls

coveralls commented Jun 16, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.3%) to 84.657% when pulling 96ca8ce on TheRealHaui:add-some-Unit-Tests into 0d8ab18 on apache:master.

Michael Hausegger added 2 commits June 16, 2017 20:22
…on in class ChecksumCalculatingInputStreamTest. Test represented a bug/defect which is going to be fixed in a different branch.
@coveralls

coveralls commented Jun 16, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.3%) to 84.673% when pulling be0f11f on TheRealHaui:add-some-Unit-Tests into 0d8ab18 on apache:master.

@coveralls

coveralls commented Jun 16, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.3%) to 84.673% when pulling 3b46bb5 on TheRealHaui:add-some-Unit-Tests into 0d8ab18 on apache:master.

@TheRealHaui

Copy link
Copy Markdown
ContributorAuthor

@bodewig
Thank you for your kind response!
Really appreciate that!

Therefore I've made all the changes you requested/proposed.
And of course added myself as a contributor as heavily requested by you.
Really couldn't disappoint you regarding that special topic. :-)

Furthermore I've created a Jira Task for the bespoken bug: https://issues.apache.org/jira/browse/COMPRESS-412
And fixed it in an own branch which I've commited, created tests for and pushed too.
And which pull request I am going to link here in the next comment after I've created it.

@TheRealHaui

Copy link
Copy Markdown
ContributorAuthor

Link to the pull request: #35

@coveralls

coveralls commented Jun 16, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.4%) to 84.713% when pulling e0500a6 on TheRealHaui:add-some-Unit-Tests into 0d8ab18 on apache:master.

@bodewig

Copy link
Copy Markdown
Member

Thanks a lot.

xZCompressorOutputStream.write(4590);

assertEquals(24, byteArrayOutputStream.size());
assertEquals("\uFFFD7zXZ\u0000\u0000\u0004\uFFFD\u05B4F\u0002\u0000!\u0001\u0016\u0000\u0000\u0000t/\uFFFD", byteArrayOutputStream.toString());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's usually not a good idea to convert an arbitrary byte array to a string, as on can't rely on a specific default charset, and you may end up with characters mangled /mapped.

I thinkThat hamcrest has array matchers; if not, assertTrue(Arrays.equals(exp,act)) should be OK.
I can't remember if there's an assertEquals for byte[]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this. I've reformulated the test with commit b53ead4 - I think we are not that much interested in the exact bytes created but rather that using write creates a valid XZ output.

JUnit's Assert class contains assertArrayEquals.

@TheRealHaui

Copy link
Copy Markdown
ContributorAuthor

@bodewig
You're welcome, was a pleasure!

Sign up for freeto 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.

4 participants

@TheRealHaui@coveralls@bodewig@sesuncedu