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

add some feature #154

Merged
merged 5 commits into from
Mar 9, 2018
Merged

add some feature #154

merged 5 commits into from
Mar 9, 2018

Conversation

cpd0101
Copy link
Contributor

@cpd0101 cpd0101 commented Dec 5, 2017

No description provided.

@howardabrams
Copy link
Collaborator

Thank you for the feature. Could you add some unit tests that verify these new functions? I would love to have the tests as part of the same PR. Thank you.

@eugef
Copy link
Owner

eugef commented Feb 9, 2018

@cpd0101 are you still interested in writing some tests for this feature?

@LoneRifle
Copy link
Contributor

I've just forked @cpd0101 's repo; I'll try to make time to flesh out the unit tests (and perhaps some documentation) and submit a PR to supersede this one in due course.

* Declare a new getter `_getChunks`, for testing purposes
* Add javadoc for  `_getBuffer`
* Add a suite of tests that ensure that calls to `write` and `end` with payloads
result in the correct state of `_data`, `_chunks` and `_buffer`. Ensure that methods
are tested in isolation, using a separate suite for interactions between `write` and `end`
@LoneRifle
Copy link
Contributor

LoneRifle commented Mar 4, 2018

PRs filed - 1 against @cpd0101's branch so that this can have tests, and 1 against master in case the tests do not get merged.

mockResponse - tests to verify `write()` and `end()` with payloads
@eugef
Copy link
Owner

eugef commented Mar 5, 2018

LGTM, @howardabrams what do you think?

@eugef eugef merged commit 8c53a59 into eugef:master Mar 9, 2018
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.

4 participants