From b89a15fcca05e821aa3617ccee2f98be4aa63263 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 7 Feb 2024 14:46:29 +0100 Subject: [PATCH] feat(webconnectivitylte): wire-in SummaryKeys (#1493) Closes https://github.com/ooni/probe/issues/2667. While there, repair flaky unit test and explain why it was flaky. --- internal/database/actions_test.go | 7 ++ .../webconnectivitylte/analysisclassic.go | 12 ++-- .../experiment/webconnectivitylte/summary.go | 32 +++++++++ .../webconnectivitylte/summary_test.go | 70 +++++++++++++++++++ .../experiment/webconnectivitylte/testkeys.go | 4 +- 5 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 internal/experiment/webconnectivitylte/summary.go create mode 100644 internal/experiment/webconnectivitylte/summary_test.go diff --git a/internal/database/actions_test.go b/internal/database/actions_test.go index be9b2612ef..97ceb008b5 100644 --- a/internal/database/actions_test.go +++ b/internal/database/actions_test.go @@ -450,6 +450,13 @@ func TestUpdateDatabaseMeasurementWithSummaryKeys(t *testing.T) { ffiller := &testingx.FakeFiller{} ffiller.Fill(sk) + // This field is not serialized, because historically it was used as an ABI to + // communicate between probe-engine and probe-cli, so we should not allow the fake + // filler to initialize it to a random value, otherwise we'll have issues in 50% + // of the cases where we would expect true and unmarshal false. For this reason, + // let's always set the field to the expected unmarshal value, i.e., false. + sk.IsAnomaly = false + if err := updateDatabaseMeasurementWithSummaryKeys(meas, sk); err != nil { t.Fatal(err) } diff --git a/internal/experiment/webconnectivitylte/analysisclassic.go b/internal/experiment/webconnectivitylte/analysisclassic.go index eae59c41b4..0db70b7c5b 100644 --- a/internal/experiment/webconnectivitylte/analysisclassic.go +++ b/internal/experiment/webconnectivitylte/analysisclassic.go @@ -148,17 +148,17 @@ func (tk *TestKeys) titleMatch() optional.Value[bool] { // setBlockingFalse implements analysisClassicTestKeysProxy. func (tk *TestKeys) setBlockingFalse() { tk.Blocking = false - tk.Accessible = true + tk.Accessible = optional.Some(true) } // setBlockingNil implements analysisClassicTestKeysProxy. func (tk *TestKeys) setBlockingNil() { if !tk.DNSConsistency.IsNone() && tk.DNSConsistency.Unwrap() == "inconsistent" { tk.Blocking = "dns" - tk.Accessible = false + tk.Accessible = optional.Some(false) } else { tk.Blocking = nil - tk.Accessible = nil + tk.Accessible = optional.None[bool]() } } @@ -169,7 +169,7 @@ func (tk *TestKeys) setBlockingString(value string) { } else { tk.Blocking = value } - tk.Accessible = false + tk.Accessible = optional.Some(false) } // setHTTPExperimentFailure implements analysisClassicTestKeysProxy. @@ -181,10 +181,10 @@ func (tk *TestKeys) setHTTPExperimentFailure(value optional.Value[string]) { func (tk *TestKeys) setWebsiteDown() { if !tk.DNSConsistency.IsNone() && tk.DNSConsistency.Unwrap() == "inconsistent" { tk.Blocking = "dns" - tk.Accessible = false + tk.Accessible = optional.Some(false) } else { tk.Blocking = false - tk.Accessible = false + tk.Accessible = optional.Some(false) } } diff --git a/internal/experiment/webconnectivitylte/summary.go b/internal/experiment/webconnectivitylte/summary.go new file mode 100644 index 0000000000..5f63e4493c --- /dev/null +++ b/internal/experiment/webconnectivitylte/summary.go @@ -0,0 +1,32 @@ +package webconnectivitylte + +import "github.com/ooni/probe-cli/v3/internal/model" + +var _ model.MeasurementSummaryKeysProvider = &TestKeys{} + +// SummaryKeys contains summary keys for this experiment. +type SummaryKeys struct { + Accessible bool `json:"accessible"` + Blocking string `json:"blocking"` + IsAnomaly bool `json:"-"` +} + +// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider. +func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys { + // TODO(https://github.com/ooni/probe/issues/1684) + sk := &SummaryKeys{} + switch v := tk.Blocking.(type) { + case string: + sk.IsAnomaly = true + sk.Blocking = v + default: + // nothing + } + sk.Accessible = tk.Accessible.UnwrapOr(false) + return sk +} + +// Anomaly implements model.MeasurementSummaryKeys. +func (sk *SummaryKeys) Anomaly() bool { + return sk.IsAnomaly +} diff --git a/internal/experiment/webconnectivitylte/summary_test.go b/internal/experiment/webconnectivitylte/summary_test.go new file mode 100644 index 0000000000..9412af9bb4 --- /dev/null +++ b/internal/experiment/webconnectivitylte/summary_test.go @@ -0,0 +1,70 @@ +package webconnectivitylte + +import ( + "testing" + + "github.com/ooni/probe-cli/v3/internal/optional" +) + +func TestSummaryKeys(t *testing.T) { + type testcase struct { + name string + accessible optional.Value[bool] + blocking any + expectAccessible bool + expectBlocking string + expectIsAnomaly bool + } + + cases := []testcase{{ + name: "with all nil", + accessible: optional.None[bool](), + blocking: nil, + expectAccessible: false, + expectBlocking: "", + expectIsAnomaly: false, + }, { + name: "with success", + accessible: optional.Some(true), + blocking: false, + expectAccessible: true, + expectBlocking: "", + expectIsAnomaly: false, + }, { + name: "with website down", + accessible: optional.Some(false), + blocking: false, + expectAccessible: false, + expectBlocking: "", + expectIsAnomaly: false, + }, { + name: "with anomaly", + accessible: optional.Some(false), + blocking: "http-failure", + expectAccessible: false, + expectBlocking: "http-failure", + expectIsAnomaly: true, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + tk := &TestKeys{ + Accessible: tc.accessible, + Blocking: tc.blocking, + } + sk := tk.MeasurementSummaryKeys().(*SummaryKeys) + if sk.Accessible != tc.expectAccessible { + t.Fatal("expected", tc.expectAccessible, "got", sk.Accessible) + } + if sk.Blocking != tc.expectBlocking { + t.Fatal("expected", tc.expectBlocking, "got", sk.Blocking) + } + if sk.IsAnomaly != tc.expectIsAnomaly { + t.Fatal("expected", tc.expectIsAnomaly, "got", sk.IsAnomaly) + } + if sk.Anomaly() != sk.IsAnomaly { + t.Fatal("expected Anomaly() to equal IsAnomaly") + } + }) + } +} diff --git a/internal/experiment/webconnectivitylte/testkeys.go b/internal/experiment/webconnectivitylte/testkeys.go index 2674836bb1..c7b16181b5 100644 --- a/internal/experiment/webconnectivitylte/testkeys.go +++ b/internal/experiment/webconnectivitylte/testkeys.go @@ -127,7 +127,7 @@ type TestKeys struct { // Accessible indicates whether the resource is accessible. Possible // values for this field are: nil, true, and false. - Accessible any `json:"accessible"` + Accessible optional.Value[bool] `json:"accessible"` // fundamentalFailure indicates that some fundamental error occurred // in a background task. A fundamental error is something like a programmer @@ -368,7 +368,7 @@ func NewTestKeys() *TestKeys { StatusCodeMatch: optional.None[bool](), TitleMatch: optional.None[bool](), Blocking: nil, - Accessible: nil, + Accessible: optional.None[bool](), ControlRequest: nil, fundamentalFailure: nil, mu: &sync.Mutex{},