-
Notifications
You must be signed in to change notification settings - Fork 16
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
Enable tests #6
Enable tests #6
Conversation
.travis.yml
Outdated
sudo: false | ||
os: | ||
- linux | ||
- osx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to test this on macos. Historically, the queue has been really long and this has no os specific code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad; I copied it from another repo without thinking too much about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(which repo? We have a bunch of outdate CI scripts...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got it from the go-maddr-filter
repo: https://github.com/libp2p/go-maddr-filter/blob/master/.travis.yml.
I can remove the osx build from there as well (and remove it from other repos that don't have OS specific code as I encounter such instances).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're willing to spend the time at it, that would be great! We usually just wait for it to sufficiently annoy someone.
|
||
install: | ||
before_install: | ||
- make deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to replace the install section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
@@ -1,7 +1,10 @@ | |||
go-buffer-pool | |||
================== | |||
|
|||
[![](https://img.shields.io/badge/made%20by-Protocol%20Labs-blue.svg?style=flat-square)](https://protocol.ai) | |||
[![](https://img.shields.io/badge/made%20by-Protocol%20Labs-blue.svg?style=flat-square)](http://protocol.ai) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theses should all be https (we probably need to normalize our repos).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thanks! |
@@ -27,7 +27,7 @@ func TestAllocations(t *testing.T) { | |||
runtime.GC() | |||
runtime.ReadMemStats(&m2) | |||
frees := m2.Frees - m1.Frees | |||
if frees > 100 { | |||
if frees > 1000 { | |||
t.Fatalf("expected less than 100 frees after GC, got %d", frees) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm this line is out of date. Seems like an unreliable test anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's timing related. Basically, I just want to make sure that the pool is actually saving us allocations.
Fixes #5 and enables code coverage metrics.