refactor: start pivoting netx (#396)

What do I mean by pivoting? Netx is currently organized by row:

```
               | dialer | quicdialer | resolver | ...
 saving        |        |            |          | ...
 errorwrapping |        |            |          | ...
 logging       |        |            |          | ...
 mocking/sys   |        |            |          | ...
```

Every row needs to implement saving, errorwrapping, logging, mocking (or
adapting to the system or to some underlying library).

This causes cross package dependencies and, in turn, complexity. For
example, we need the `trace` package for supporting saving.

And `dialer`, `quickdialer`, et al. need to depend on such a package.

The same goes for errorwrapping.

This arrangement further complicates testing. For example, I am
currently working on https://github.com/ooni/probe/issues/1505 and
I realize it need to repeat integration tests in multiple places.

Let's say instead we pivot the above matrix as follows:

```
             | saving | errorwrapping | logging | ...
 dialer      |        |               |         | ...
 quicdialer  |        |               |         | ...
 logging     |        |               |         | ...
 mocking/sys |        |               |         | ...
 ...
```

In this way, now every row contains everything related to a specific
action to perform. We can now share code without relying on extra
support packages. What's more, we can write tests and, judding from
the way in which things are made, it seems we only need integration
testing in `errorwrapping` because it's where data quality matters
whereas, in all other cases, unit testing is fine.

I am going, therefore, to proceed with these changes and "pivot"
`netx`. Hopefully, it won't be too painful.
This commit is contained in:
Simone Basso
2021-06-23 15:53:12 +02:00
committed by GitHub
parent c74c94d616
commit 8a0beee808
27 changed files with 457 additions and 353 deletions
+2 -1
View File
@@ -5,12 +5,13 @@ import (
"testing"
"github.com/ooni/probe-cli/v3/internal/engine/netx/resolver"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)
func TestChainLookupHost(t *testing.T) {
r := resolver.ChainResolver{
Primary: resolver.NewFakeResolverThatFails(),
Secondary: resolver.SystemResolver{},
Secondary: netxlite.ResolverSystem{},
}
if r.Address() != "" {
t.Fatal("invalid address")
@@ -8,6 +8,7 @@ import (
"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/engine/netx/resolver"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)
func init() {
@@ -18,7 +19,7 @@ func testresolverquick(t *testing.T, reso resolver.Resolver) {
if testing.Short() {
t.Skip("skip test in short mode")
}
reso = resolver.LoggingResolver{Logger: log.Log, Resolver: reso}
reso = netxlite.ResolverLogger{Logger: log.Log, Resolver: reso}
addrs, err := reso.LookupHost(context.Background(), "dns.google.com")
if err != nil {
t.Fatal(err)
@@ -44,7 +45,7 @@ func testresolverquickidna(t *testing.T, reso resolver.Resolver) {
t.Skip("skip test in short mode")
}
reso = resolver.IDNAResolver{
resolver.LoggingResolver{Logger: log.Log, Resolver: reso},
netxlite.ResolverLogger{Logger: log.Log, Resolver: reso},
}
addrs, err := reso.LookupHost(context.Background(), "яндекс.рф")
if err != nil {
@@ -56,7 +57,7 @@ func testresolverquickidna(t *testing.T, reso resolver.Resolver) {
}
func TestNewResolverSystem(t *testing.T) {
reso := resolver.SystemResolver{}
reso := netxlite.ResolverSystem{}
testresolverquick(t, reso)
testresolverquickidna(t, reso)
}
-30
View File
@@ -1,30 +0,0 @@
package resolver
import (
"context"
"time"
)
// Logger is the logger assumed by this package
type Logger interface {
Debugf(format string, v ...interface{})
Debug(message string)
}
// LoggingResolver is a resolver that emits events
type LoggingResolver struct {
Resolver
Logger Logger
}
// LookupHost returns the IP addresses of a host
func (r LoggingResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
r.Logger.Debugf("resolve %s...", hostname)
start := time.Now()
addrs, err := r.Resolver.LookupHost(ctx, hostname)
stop := time.Now()
r.Logger.Debugf("resolve %s... (%+v, %+v) in %s", hostname, addrs, err, stop.Sub(start))
return addrs, err
}
var _ Resolver = LoggingResolver{}
@@ -1,23 +0,0 @@
package resolver_test
import (
"context"
"testing"
"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/engine/netx/resolver"
)
func TestLoggingResolver(t *testing.T) {
r := resolver.LoggingResolver{
Logger: log.Log,
Resolver: resolver.NewFakeResolverThatFails(),
}
addrs, err := r.LookupHost(context.Background(), "www.google.com")
if err == nil {
t.Fatal("expected an error here")
}
if addrs != nil {
t.Fatal("expected nil addr here")
}
}
-29
View File
@@ -1,29 +0,0 @@
package resolver
import (
"context"
"net"
)
// SystemResolver is the system resolver.
type SystemResolver struct{}
// LookupHost implements Resolver.LookupHost.
func (r SystemResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
return net.DefaultResolver.LookupHost(ctx, hostname)
}
// Network implements Resolver.Network.
func (r SystemResolver) Network() string {
return "system"
}
// Address implements Resolver.Address.
func (r SystemResolver) Address() string {
return ""
}
// Default is the resolver we use by default.
var Default = SystemResolver{}
var _ Resolver = SystemResolver{}
@@ -1,25 +0,0 @@
package resolver_test
import (
"context"
"testing"
"github.com/ooni/probe-cli/v3/internal/engine/netx/resolver"
)
func TestSystemResolverLookupHost(t *testing.T) {
r := resolver.SystemResolver{}
if r.Network() != "system" {
t.Fatal("invalid Network")
}
if r.Address() != "" {
t.Fatal("invalid Address")
}
addrs, err := r.LookupHost(context.Background(), "dns.google.com")
if err != nil {
t.Fatal(err)
}
if addrs == nil {
t.Fatal("expected non-nil result here")
}
}