Skip to content

Commit

Permalink
refactor(webconnectivity*): don't call httpapi directly (#1582)
Browse files Browse the repository at this point in the history 
The #1581 pull request missed that
adding a `CallWebConnectivityTestHelper` method to `*engine.Session`
means we need to mock such a method in `webconnectivityqa`.

I could have mocked the method, but I was not super happy about doing
this, because I would rather have wanted to add more QA tests inside of
`webconnectivityqa` making sure we're indeed falling back when using
test helpers. (We have unit tests testing that, but it would be nice to
have integration tests, so we could observe the overall code behavior.)

My initial approach for allowing this was to create a
`CallWebConnectivityTestHelper` function inside the engine, taking a
`model.ExperimentSession` as its fourth argument and then modifying the
`CallWebConnectivityTestHelper` method to just invoke the function
passing to it the session. However, this approach created import cycles
in tests.

To avoid import cycles, I moved the `CallWebConnectivityTestHelper`
function to `webconnectivityalgo` and modified the mocks inside
`webconnectivityqa` to call such a function. This is where I paused and
realized that the cleanest solution is actually to just always call the
`CallWebConnectivityTestHelper` function in `webconnectivityalgo` and
undo part of the changes in #1581
to avoid having a `CallWebConnectivityTestHelper` method inside of
`model.ExperimentSession`. And so I also implemented this change.

All of this leads us to the current diff, which is part of
ooni/probe#2725.
  • Loading branch information
@bassosimone
bassosimone committed May 2, 2024
1 parent 9a3abfc commit cc25204
Show file tree
Hide file tree
Showing 10 changed files with 363 additions and 404 deletions.
32 changes: 0 additions & 32 deletions internal/engine/session.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@ import (
"github.com/ooni/probe-cli/v3/internal/enginelocate"
"github.com/ooni/probe-cli/v3/internal/enginenetx"
"github.com/ooni/probe-cli/v3/internal/engineresolver"
"github.com/ooni/probe-cli/v3/internal/httpapi"
"github.com/ooni/probe-cli/v3/internal/kvstore"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/ooapi"
"github.com/ooni/probe-cli/v3/internal/platform"
"github.com/ooni/probe-cli/v3/internal/probeservices"
"github.com/ooni/probe-cli/v3/internal/runtimex"
Expand Down Expand Up @@ -690,34 +688,4 @@ func (s *Session) MaybeLookupLocationContext(ctx context.Context) error {
return nil
}

// CallWebConnectivityTestHelper implements [model.EngineExperimentSession].
func (s *Session) CallWebConnectivityTestHelper(ctx context.Context,
creq *model.THRequest, testhelpers []model.OOAPIService) (*model.THResponse, int, error) {
// handle the case where there are no available web connectivity test helpers
if len(testhelpers) <= 0 {
return nil, 0, model.ErrNoAvailableTestHelpers
}

// initialize a sequence caller for invoking the THs in FIFO order
seqCaller := httpapi.NewSequenceCaller(
ooapi.NewDescriptorTH(creq),
httpapi.NewEndpointList(s.DefaultHTTPClient(), s.Logger(), s.UserAgent(), testhelpers...)...,
)

// issue the composed call proper and obtain a response and an index or an error
cresp, idx, err := seqCaller.Call(ctx)

// handle the case where all test helpers failed
if err != nil {
return nil, 0, err
}

// apply some sanity checks to the results
runtimex.Assert(idx >= 0 && idx < len(testhelpers), "idx out of bounds")
runtimex.Assert(cresp != nil, "out is nil")

// return the results to the web connectivity caller
return cresp, idx, nil
}

var _ model.ExperimentSession = &Session{}
303 changes: 0 additions & 303 deletions internal/engine/session_internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,20 @@ package engine
import (
"context"
"errors"
"net/http"
"net/url"
"sync"
"testing"
"time"

"github.com/apex/log"
"github.com/google/go-cmp/cmp"
"github.com/ooni/probe-cli/v3/internal/bytecounter"
"github.com/ooni/probe-cli/v3/internal/checkincache"
"github.com/ooni/probe-cli/v3/internal/enginelocate"
"github.com/ooni/probe-cli/v3/internal/enginenetx"
"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivity"
"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte"
"github.com/ooni/probe-cli/v3/internal/kvstore"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
"github.com/ooni/probe-cli/v3/internal/registry"
"github.com/ooni/probe-cli/v3/internal/testingx"
"github.com/ooni/probe-cli/v3/internal/version"
)

func (s *Session) GetAvailableProbeServices() []model.OOAPIService {
Expand Down Expand Up @@ -411,300 +405,3 @@ func TestSessionNewExperimentBuilder(t *testing.T) {
}
})
}

// This function tests the [*Session.CallWebConnectivityTestHelper] method.
func TestSessionCallWebConnectivityTestHelper(t *testing.T) {
// We start with simple tests that exercise the basic functionality of the method
// without bothering with having more than one available test helper.

t.Run("when there are no available test helpers", func(t *testing.T) {
// create a new session only initializing the fields that
// are going to matter for running this specific test
sess := &Session{
network: enginenetx.NewNetwork(
bytecounter.New(),
&kvstore.Memory{},
model.DiscardLogger,
nil,
(&netxlite.Netx{}).NewStdlibResolver(model.DiscardLogger),
),
logger: model.DiscardLogger,
softwareName: "miniooni",
softwareVersion: version.Version,
}

// create a new background context
ctx := context.Background()

// create a fake request for the test helper
//
// note: no need to fill the request for this test case
creq := &model.THRequest{}

// invoke the API
cresp, idx, err := sess.CallWebConnectivityTestHelper(ctx, creq, nil)

// make sure we get the expected error
if !errors.Is(err, model.ErrNoAvailableTestHelpers) {
t.Fatal("unexpected error", err)
}

// make sure idx is zero
if idx != 0 {
t.Fatal("expected zero, got", idx)
}

// make sure cresp is nil
if cresp != nil {
t.Fatal("expected nil, got", cresp)
}
})

t.Run("when the call fails", func(t *testing.T) {
// create a local test server that always resets the connection
server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset())
defer server.Close()

// create a new session only initializing the fields that
// are going to matter for running this specific test
sess := &Session{
network: enginenetx.NewNetwork(
bytecounter.New(),
&kvstore.Memory{},
model.DiscardLogger,
nil,
(&netxlite.Netx{}).NewStdlibResolver(model.DiscardLogger),
),
logger: model.DiscardLogger,
softwareName: "miniooni",
softwareVersion: version.Version,
}

// create a new background context
ctx := context.Background()

// create a fake request for the test helper
//
// note: no need to fill the request for this test case
creq := &model.THRequest{}

// create the list of test helpers to use
testhelpers := []model.OOAPIService{{
Address: server.URL,
Type: "https",
Front: "",
}}

// invoke the API
cresp, idx, err := sess.CallWebConnectivityTestHelper(ctx, creq, testhelpers)

// make sure we get the expected error
if !errors.Is(err, netxlite.ECONNRESET) {
t.Fatal("unexpected error", err)
}

// make sure idx is zero
if idx != 0 {
t.Fatal("expected zero, got", idx)
}

// make sure cresp is nil
if cresp != nil {
t.Fatal("expected nil, got", cresp)
}
})

t.Run("when the call succeeds", func(t *testing.T) {
// create a local test server that always returns an ~empty response
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(`{}`))
}))
defer server.Close()

// create a new session only initializing the fields that
// are going to matter for running this specific test
sess := &Session{
network: enginenetx.NewNetwork(
bytecounter.New(),
&kvstore.Memory{},
model.DiscardLogger,
nil,
(&netxlite.Netx{}).NewStdlibResolver(model.DiscardLogger),
),
logger: model.DiscardLogger,
softwareName: "miniooni",
softwareVersion: version.Version,
}

// create a new background context
ctx := context.Background()

// create a fake request for the test helper
//
// note: no need to fill the request for this test case
creq := &model.THRequest{}

// create the list of test helpers to use
testhelpers := []model.OOAPIService{{
Address: server.URL,
Type: "https",
Front: "",
}}

// invoke the API
cresp, idx, err := sess.CallWebConnectivityTestHelper(ctx, creq, testhelpers)

// make sure we get the expected error
if err != nil {
t.Fatal("unexpected error", err)
}

// make sure idx is zero
if idx != 0 {
t.Fatal("expected zero, got", idx)
}

// make sure cresp is not nil
if cresp == nil {
t.Fatal("expected not nil, got", cresp)
}
})

t.Run("with two test helpers where the first one resets the connection and the second works", func(t *testing.T) {
// create a local test server1 that always resets the connection
server1 := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset())
defer server1.Close()

// create a local test server2 that always returns an ~empty response
server2 := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(`{}`))
}))
defer server2.Close()

// create a new session only initializing the fields that
// are going to matter for running this specific test
sess := &Session{
network: enginenetx.NewNetwork(
bytecounter.New(),
&kvstore.Memory{},
model.DiscardLogger,
nil,
(&netxlite.Netx{}).NewStdlibResolver(model.DiscardLogger),
),
logger: model.DiscardLogger,
softwareName: "miniooni",
softwareVersion: version.Version,
}

// create a new background context
ctx := context.Background()

// create a fake request for the test helper
//
// note: no need to fill the request for this test case
creq := &model.THRequest{}

// create the list of test helpers to use
testhelpers := []model.OOAPIService{{
Address: server1.URL,
Type: "https",
Front: "",
}, {
Address: server2.URL,
Type: "https",
Front: "",
}}

// invoke the API
cresp, idx, err := sess.CallWebConnectivityTestHelper(ctx, creq, testhelpers)

// make sure we get the expected error
if err != nil {
t.Fatal("unexpected error", err)
}

// make sure idx is one
if idx != 1 {
t.Fatal("expected one, got", idx)
}

// make sure cresp is not nil
if cresp == nil {
t.Fatal("expected not nil, got", cresp)
}
})

t.Run("with two test helpers where the first one times out the connection and the second works", func(t *testing.T) {
// TODO(bassosimone): the utility of this test will become more obvious
// once we switch this specific test to using httpclientx.

// create a local test server1 that resets the connection after a ~long delay
server1 := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
select {
case <-time.After(10 * time.Second):
testingx.HTTPHandlerReset().ServeHTTP(w, r)
case <-r.Context().Done():
return
}
}))
defer server1.Close()

// create a local test server2 that always returns an ~empty response
server2 := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(`{}`))
}))
defer server2.Close()

// create a new session only initializing the fields that
// are going to matter for running this specific test
sess := &Session{
network: enginenetx.NewNetwork(
bytecounter.New(),
&kvstore.Memory{},
model.DiscardLogger,
nil,
(&netxlite.Netx{}).NewStdlibResolver(model.DiscardLogger),
),
logger: model.DiscardLogger,
softwareName: "miniooni",
softwareVersion: version.Version,
}

// create a new background context
ctx := context.Background()

// create a fake request for the test helper
//
// note: no need to fill the request for this test case
creq := &model.THRequest{}

// create the list of test helpers to use
testhelpers := []model.OOAPIService{{
Address: server1.URL,
Type: "https",
Front: "",
}, {
Address: server2.URL,
Type: "https",
Front: "",
}}

// invoke the API
cresp, idx, err := sess.CallWebConnectivityTestHelper(ctx, creq, testhelpers)

// make sure we get the expected error
if err != nil {
t.Fatal("unexpected error", err)
}

// make sure idx is one
if idx != 1 {
t.Fatal("expected one, got", idx)
}

// make sure cresp is not nil
if cresp == nil {
t.Fatal("expected not nil, got", cresp)
}
})
}

0 comments on commit cc25204

Please sign in to comment.