From 87e52345860532845e0b120965802778be8b1a65 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 29 Mar 2021 19:19:06 +0200 Subject: [PATCH] refactor(inputloader): all fast tests together (#269) This change allows us to have all fasts tests together. They are mostly unit tests or integration tests that do not require the network. The advantage of this strategy is the following. We can now run all these tests with a single click in VSCode. In turn, doing that tells us which lines of code we are not covering. The tests requiring the network are in a separate file, so we can easily see which lines of code are testing without using the network and which ones instead depend on that. (Currently, 100% of the inputloader.go file is tested without using the network.) While there, rename the other file such that is clear that it contains tests requiring the network. We now have some tests in inputloader_test.go that are not strictly unit tests. This refactoring was identified as useful while working on https://github.com/ooni/probe/issues/1299. --- .../engine/inputloader_integration_test.go | 316 ------------------ internal/engine/inputloader_network_test.go | 46 +++ internal/engine/inputloader_test.go | 269 +++++++++++++++ 3 files changed, 315 insertions(+), 316 deletions(-) delete mode 100644 internal/engine/inputloader_integration_test.go create mode 100644 internal/engine/inputloader_network_test.go diff --git a/internal/engine/inputloader_integration_test.go b/internal/engine/inputloader_integration_test.go deleted file mode 100644 index 1662746..0000000 --- a/internal/engine/inputloader_integration_test.go +++ /dev/null @@ -1,316 +0,0 @@ -package engine_test - -import ( - "context" - "errors" - "syscall" - "testing" - - "github.com/apex/log" - "github.com/google/go-cmp/cmp" - engine "github.com/ooni/probe-cli/v3/internal/engine" - "github.com/ooni/probe-cli/v3/internal/engine/kvstore" - "github.com/ooni/probe-cli/v3/internal/engine/model" -) - -func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - StaticInputs: []string{"https://www.google.com/"}, - InputPolicy: engine.InputNone, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if !errors.Is(err, engine.ErrNoInputExpected) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", - }, - InputPolicy: engine.InputNone, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if !errors.Is(err, engine.ErrNoInputExpected) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - StaticInputs: []string{"https://www.google.com/"}, - SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", - }, - InputPolicy: engine.InputNone, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if !errors.Is(err, engine.ErrNoInputExpected) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputNoneWithNoInput(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputNone, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if err != nil { - t.Fatal(err) - } - if len(out) != 1 || out[0].URL != "" { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOptional, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if err != nil { - t.Fatal(err) - } - if len(out) != 1 || out[0].URL != "" { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputOptionalWithInput(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - StaticInputs: []string{"https://www.google.com/"}, - SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", - }, - InputPolicy: engine.InputOptional, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if err != nil { - t.Fatal(err) - } - if len(out) != 5 { - t.Fatal("not the output length we expected") - } - expect := []model.URLInfo{ - {URL: "https://www.google.com/"}, - {URL: "https://www.x.org/"}, - {URL: "https://www.slashdot.org/"}, - {URL: "https://abc.xyz/"}, - {URL: "https://run.ooni.io/"}, - } - if diff := cmp.Diff(out, expect); diff != "" { - t.Fatal(diff) - } -} - -func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - StaticInputs: []string{"https://www.google.com/"}, - SourceFiles: []string{ - "testdata/inputloader1.txt", - "/nonexistent", - "testdata/inputloader2.txt", - }, - InputPolicy: engine.InputOptional, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if !errors.Is(err, syscall.ENOENT) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - StaticInputs: []string{"https://www.google.com/"}, - SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", - }, - InputPolicy: engine.InputStrictlyRequired, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if err != nil { - t.Fatal(err) - } - if len(out) != 5 { - t.Fatal("not the output length we expected") - } - expect := []model.URLInfo{ - {URL: "https://www.google.com/"}, - {URL: "https://www.x.org/"}, - {URL: "https://www.slashdot.org/"}, - {URL: "https://abc.xyz/"}, - {URL: "https://run.ooni.io/"}, - } - if diff := cmp.Diff(out, expect); diff != "" { - t.Fatal(diff) - } -} - -func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputStrictlyRequired, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if !errors.Is(err, engine.ErrInputRequired) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputStrictlyRequired, - SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader3.txt", // we want it before inputloader2.txt - "testdata/inputloader2.txt", - }, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if !errors.Is(err, engine.ErrDetectedEmptyFile) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - StaticInputs: []string{"https://www.google.com/"}, - SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", - }, - InputPolicy: engine.InputOrQueryBackend, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if err != nil { - t.Fatal(err) - } - if len(out) != 5 { - t.Fatal("not the output length we expected") - } - expect := []model.URLInfo{ - {URL: "https://www.google.com/"}, - {URL: "https://www.x.org/"}, - {URL: "https://www.slashdot.org/"}, - {URL: "https://abc.xyz/"}, - {URL: "https://run.ooni.io/"}, - } - if diff := cmp.Diff(out, expect); diff != "" { - t.Fatal(diff) - } -} - -func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing.T) { - sess, err := engine.NewSession(engine.SessionConfig{ - AssetsDir: "testdata", - KVStore: kvstore.NewMemoryKeyValueStore(), - Logger: log.Log, - SoftwareName: "miniooni", - SoftwareVersion: "0.1.0-dev", - TempDir: "testdata", - }) - if err != nil { - t.Fatal(err) - } - defer sess.Close() - il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOrQueryBackend, - Session: sess, - }) - ctx, cancel := context.WithCancel(context.Background()) - cancel() // fail immediately - out, err := il.Load(ctx) - if !errors.Is(err, context.Canceled) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} - -func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess, err := engine.NewSession(engine.SessionConfig{ - AvailableProbeServices: []model.Service{{ - Address: "https://ams-pg-test.ooni.org/", - Type: "https", - }}, - AssetsDir: "testdata", - KVStore: kvstore.NewMemoryKeyValueStore(), - Logger: log.Log, - SoftwareName: "miniooni", - SoftwareVersion: "0.1.0-dev", - TempDir: "testdata", - }) - if err != nil { - t.Fatal(err) - } - defer sess.Close() - il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOrQueryBackend, - Session: sess, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if err != nil { - t.Fatal(err) - } - if len(out) < 10 { - // check-in SHOULD return AT LEAST 20 URLs at a time. - t.Fatal("not the output length we expected") - } -} - -func TestInputLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { - il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOrQueryBackend, - SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader3.txt", // we want it before inputloader2.txt - "testdata/inputloader2.txt", - }, - }) - ctx := context.Background() - out, err := il.Load(ctx) - if !errors.Is(err, engine.ErrDetectedEmptyFile) { - t.Fatalf("not the error we expected: %+v", err) - } - if out != nil { - t.Fatal("not the output we expected") - } -} diff --git a/internal/engine/inputloader_network_test.go b/internal/engine/inputloader_network_test.go new file mode 100644 index 0000000..7155d6d --- /dev/null +++ b/internal/engine/inputloader_network_test.go @@ -0,0 +1,46 @@ +package engine_test + +import ( + "context" + "testing" + + "github.com/apex/log" + engine "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/engine/kvstore" + "github.com/ooni/probe-cli/v3/internal/engine/model" +) + +func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + sess, err := engine.NewSession(engine.SessionConfig{ + AvailableProbeServices: []model.Service{{ + Address: "https://ams-pg-test.ooni.org/", + Type: "https", + }}, + AssetsDir: "testdata", + KVStore: kvstore.NewMemoryKeyValueStore(), + Logger: log.Log, + SoftwareName: "miniooni", + SoftwareVersion: "0.1.0-dev", + TempDir: "testdata", + }) + if err != nil { + t.Fatal(err) + } + defer sess.Close() + il := engine.NewInputLoader(engine.InputLoaderConfig{ + InputPolicy: engine.InputOrQueryBackend, + Session: sess, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if err != nil { + t.Fatal(err) + } + if len(out) < 10 { + // check-in SHOULD return AT LEAST 20 URLs at a time. + t.Fatal("not the output length we expected") + } +} diff --git a/internal/engine/inputloader_test.go b/internal/engine/inputloader_test.go index 43bfd47..7dbf9b1 100644 --- a/internal/engine/inputloader_test.go +++ b/internal/engine/inputloader_test.go @@ -8,11 +8,280 @@ import ( "syscall" "testing" + "github.com/apex/log" "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/engine/internal/fsx" + "github.com/ooni/probe-cli/v3/internal/engine/kvstore" "github.com/ooni/probe-cli/v3/internal/engine/model" ) +func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + StaticInputs: []string{"https://www.google.com/"}, + InputPolicy: InputNone, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if !errors.Is(err, ErrNoInputExpected) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + SourceFiles: []string{ + "testdata/inputloader1.txt", + "testdata/inputloader2.txt", + }, + InputPolicy: InputNone, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if !errors.Is(err, ErrNoInputExpected) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + StaticInputs: []string{"https://www.google.com/"}, + SourceFiles: []string{ + "testdata/inputloader1.txt", + "testdata/inputloader2.txt", + }, + InputPolicy: InputNone, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if !errors.Is(err, ErrNoInputExpected) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputNoneWithNoInput(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + InputPolicy: InputNone, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if err != nil { + t.Fatal(err) + } + if len(out) != 1 || out[0].URL != "" { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + InputPolicy: InputOptional, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if err != nil { + t.Fatal(err) + } + if len(out) != 1 || out[0].URL != "" { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputOptionalWithInput(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + StaticInputs: []string{"https://www.google.com/"}, + SourceFiles: []string{ + "testdata/inputloader1.txt", + "testdata/inputloader2.txt", + }, + InputPolicy: InputOptional, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if err != nil { + t.Fatal(err) + } + if len(out) != 5 { + t.Fatal("not the output length we expected") + } + expect := []model.URLInfo{ + {URL: "https://www.google.com/"}, + {URL: "https://www.x.org/"}, + {URL: "https://www.slashdot.org/"}, + {URL: "https://abc.xyz/"}, + {URL: "https://run.ooni.io/"}, + } + if diff := cmp.Diff(out, expect); diff != "" { + t.Fatal(diff) + } +} + +func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + StaticInputs: []string{"https://www.google.com/"}, + SourceFiles: []string{ + "testdata/inputloader1.txt", + "/nonexistent", + "testdata/inputloader2.txt", + }, + InputPolicy: InputOptional, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if !errors.Is(err, syscall.ENOENT) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + StaticInputs: []string{"https://www.google.com/"}, + SourceFiles: []string{ + "testdata/inputloader1.txt", + "testdata/inputloader2.txt", + }, + InputPolicy: InputStrictlyRequired, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if err != nil { + t.Fatal(err) + } + if len(out) != 5 { + t.Fatal("not the output length we expected") + } + expect := []model.URLInfo{ + {URL: "https://www.google.com/"}, + {URL: "https://www.x.org/"}, + {URL: "https://www.slashdot.org/"}, + {URL: "https://abc.xyz/"}, + {URL: "https://run.ooni.io/"}, + } + if diff := cmp.Diff(out, expect); diff != "" { + t.Fatal(diff) + } +} + +func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + InputPolicy: InputStrictlyRequired, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if !errors.Is(err, ErrInputRequired) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + InputPolicy: InputStrictlyRequired, + SourceFiles: []string{ + "testdata/inputloader1.txt", + "testdata/inputloader3.txt", // we want it before inputloader2.txt + "testdata/inputloader2.txt", + }, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if !errors.Is(err, ErrDetectedEmptyFile) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + StaticInputs: []string{"https://www.google.com/"}, + SourceFiles: []string{ + "testdata/inputloader1.txt", + "testdata/inputloader2.txt", + }, + InputPolicy: InputOrQueryBackend, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if err != nil { + t.Fatal(err) + } + if len(out) != 5 { + t.Fatal("not the output length we expected") + } + expect := []model.URLInfo{ + {URL: "https://www.google.com/"}, + {URL: "https://www.x.org/"}, + {URL: "https://www.slashdot.org/"}, + {URL: "https://abc.xyz/"}, + {URL: "https://run.ooni.io/"}, + } + if diff := cmp.Diff(out, expect); diff != "" { + t.Fatal(diff) + } +} + +func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing.T) { + sess, err := NewSession(SessionConfig{ + AssetsDir: "testdata", + KVStore: kvstore.NewMemoryKeyValueStore(), + Logger: log.Log, + SoftwareName: "miniooni", + SoftwareVersion: "0.1.0-dev", + TempDir: "testdata", + }) + if err != nil { + t.Fatal(err) + } + defer sess.Close() + il := NewInputLoader(InputLoaderConfig{ + InputPolicy: InputOrQueryBackend, + Session: sess, + }) + ctx, cancel := context.WithCancel(context.Background()) + cancel() // fail immediately + out, err := il.Load(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + +func TestInputLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { + il := NewInputLoader(InputLoaderConfig{ + InputPolicy: InputOrQueryBackend, + SourceFiles: []string{ + "testdata/inputloader1.txt", + "testdata/inputloader3.txt", // we want it before inputloader2.txt + "testdata/inputloader2.txt", + }, + }) + ctx := context.Background() + out, err := il.Load(ctx) + if !errors.Is(err, ErrDetectedEmptyFile) { + t.Fatalf("not the error we expected: %+v", err) + } + if out != nil { + t.Fatal("not the output we expected") + } +} + type InputLoaderBrokenFS struct{} func (InputLoaderBrokenFS) Open(filepath string) (fsx.File, error) {