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 test coverage to pkg/jsonmessage #13661

Merged
merged 1 commit into from
Jun 2, 2015

Conversation

vdemeester
Copy link
Member

Add coverage to pkg/jsonmessage 🐸.

# before
+ go test -test.timeout=30m github.com/docker/docker/pkg/jsonmessage
PASS
coverage: 26.9% of statements
# after
+ go test -test.timeout=30m github.com/docker/docker/pkg/jsonmessage
PASS
coverage: 96.2% of statements

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester vdemeester force-pushed the pkg-jsonmessage-test-coverage branch 2 times, most recently from c766de1 to 4c2684f Compare June 2, 2015 12:29
@estesp
Copy link
Contributor

estesp commented Jun 2, 2015

Nice to see all the new tests being added.. LGTM

@runcom
Copy link
Member

runcom commented Jun 2, 2015

LGTM, ping @duglin

jp3 := JSONProgress{Current: 20, Total: 100, Start: time.Now().Unix()}
// Just look at the start of the string
// (the remaining time is really hard to test -_-)
if jp3.String()[:len(expectedStart)] != expectedStart {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really questioning why you're only testing the start of the string, but I am curious... why does the 50% test (next) not have the same issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well no, because I've set up Start that is not set up for the 50%. I'm testing the start because I found it really hard to compute the rest of the message (the remaining time). I could give it another try though :P

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester force-pushed the pkg-jsonmessage-test-coverage branch from 4c2684f to e6bd8c1 Compare June 2, 2015 19:19
@duglin
Copy link
Contributor

duglin commented Jun 2, 2015

LGTM

duglin added a commit that referenced this pull request Jun 2, 2015
@duglin duglin merged commit da255ec into moby:master Jun 2, 2015
@vdemeester vdemeester deleted the pkg-jsonmessage-test-coverage branch June 2, 2015 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants