add banlist and tests #94
Conversation
http/banlist/banlist_test.go
Outdated
| ) | ||
|
|
||
| func init() { | ||
| logrus.SetLevel(logrus.DebugLevel) |
There was a problem hiding this comment.
If we are not using the global logrus instance in banlist.go, why do we need to set the level here? Just do it on the logger tl instead so you do not modify the global one (which could be used by another tests, etc)
There was a problem hiding this comment.
We are using the global logrus instance in testing, I'm not sure if there is a way to actually set this from env vars automatically (😭). That is then injected into the banlist.
I think it might be overkill to start a new logger each time.
There was a problem hiding this comment.
I can't see still why we use the global one. At the end you can translate all the log initialization of this test to
l := logrus.New()
l.SetLevel(logrus.DebugLevel)
l.WithField("test", t.Name())There was a problem hiding this comment.
I think it doesn't really matter at all actually. I truly don't care. Global | not in tests is not a big deal for me. If you're testing log lines and hooks you should init a new one there. So there isn't a big impact either way
http/banlist/banlist_test.go
Outdated
| } | ||
|
|
||
| func tl(t *testing.T) logrus.FieldLogger { | ||
| return logrus.WithField("test", t.Name()) |
There was a problem hiding this comment.
| return logrus.WithField("test", t.Name()) | |
| return logrus.New().WithField("test", t.Name()) |
Otherwise you are modifying the global instance. We can't ensure no one else is using it.
There was a problem hiding this comment.
so, we want to use the global one so that we can globally set the level|format
| func TestBanlistBanning(t *testing.T) { | ||
| bl := testList(t, &Config{ | ||
| URLs: []string{"villians.com/the/joker"}, | ||
| Domains: []string{"sick.com"}, |
http/banlist/banlist_test.go
Outdated
| Domains: []string{"something.com"}, | ||
| }) | ||
|
|
||
| assert.Len(t, bl.urls(), 0) |
There was a problem hiding this comment.
Minor:
| assert.Len(t, bl.urls(), 0) | |
| assert.Empty(t, bl.urls()) |
http/banlist/banlist.go
Outdated
| path string | ||
| } | ||
|
|
||
| func New(log logrus.FieldLogger, path string) *Banlist { |
There was a problem hiding this comment.
Minor:
| func New(log logrus.FieldLogger, path string) *Banlist { | |
| func New(log logrus.FieldLogger, filepath string) *Banlist { |
It confuses me the fact that I did not know if we were talking about a URL path or a filepath.
| } | ||
|
|
||
| func (b *Banlist) Close() { | ||
| signal.Stop(b.ch) |
There was a problem hiding this comment.
This is a port of what is in the proxy. I wanted to add it to commons so we could export it to other projects (e.g. nfsvr). Also I added tests and the ability to ban per path.