fix(webconnectivitylte): never sort test keys (ooni#1506)

I realized it was not the best idea to modify the measurement algorithm for producing a better test suite. With a small refactor, I can make it such we only perform these changes when we need. Then, I need to regenerate all the files, which comes with some churn. At this point, it's sad but also fine. I'll try to improve things a bit more from now one. Part of ooni/probe#2677
Murphy-OrangeMud · Feb 13, 2024 · abe9779 · abe9779
1 parent 56e034f
commit abe9779
Show file tree

Hide file tree

Showing 177 changed files with 9,683 additions and 9,849 deletions.
diff --git a/internal/cmd/qatool/main.go b/internal/cmd/qatool/main.go
@@ -65,6 +65,20 @@ func runWebConnectivityLTE(tc *webconnectivityqa.TestCase) {
 		// run the test case
 		measurement := runtimex.Try1(webconnectivityqa.MeasureTestCase(measurer, tc))
 
+		// obtain the test keys
+		tk := measurement.TestKeys.(*webconnectivitylte.TestKeys)
+
+		// normalize the test keys
+		//
+		// see https://github.com/ooni/probe/issues/2677
+		tk.Queries = minipipeline.SortDNSLookupResults(tk.Queries)
+		tk.Do53.Queries = minipipeline.SortDNSLookupResults(tk.Do53.Queries)
+		tk.DoH.Queries = minipipeline.SortDNSLookupResults(tk.DoH.Queries)
+		tk.DNSDuplicateResponses = minipipeline.SortDNSLookupResults(tk.DNSDuplicateResponses)
+		tk.NetworkEvents = minipipeline.SortNetworkEvents(tk.NetworkEvents)
+		tk.TCPConnect = minipipeline.SortTCPConnectResults(tk.TCPConnect)
+		tk.TLSHandshakes = minipipeline.SortTLSHandshakeResults(tk.TLSHandshakes)
+
 		// serialize the original measurement
 		mustSerializeMkdirAllAndWriteFile(actualDestdir, "measurement.json", measurement)
 	}
@@ -135,9 +149,6 @@ func main() {
 	// build the regexp
 	selector := regexp.MustCompile(*runFlag)
 
-	// make sure we produce more predictable observations in output
-	webconnectivitylte.SortObservations.Add(1)
-
 	// select which test cases to run
 	for _, tc := range webconnectivityqa.AllTestCases() {
 		name := "webconnectivitylte/" + tc.Name

diff --git a/internal/experiment/webconnectivitylte/measurer.go b/internal/experiment/webconnectivitylte/measurer.go
@@ -13,7 +13,6 @@ import (
 
 	"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivity"
 	"github.com/ooni/probe-cli/v3/internal/inputparser"
-	"github.com/ooni/probe-cli/v3/internal/minipipeline"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"golang.org/x/net/publicsuffix"
 )
@@ -145,23 +144,6 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 		}
 	}
 
-	// sort test keys to make output more predictable and avoid churn when generating
-	// minipipeline test cases; see https://github.com/ooni/probe/issues/2677.
-	//
-	// Note that tk.Do53 and tk.DoH are initialized by NewTestKeys so we know they're not nil.
-	//
-	// Note that we MUST NOT sort tk.Requests because its order matters for historical
-	// reasons and we don't wnat to break existing data consumers.
-	if SortObservations.Load() > 0 {
-		tk.Queries = minipipeline.SortDNSLookupResults(tk.Queries)
-		tk.Do53.Queries = minipipeline.SortDNSLookupResults(tk.Do53.Queries)
-		tk.DoH.Queries = minipipeline.SortDNSLookupResults(tk.DoH.Queries)
-		tk.DNSDuplicateResponses = minipipeline.SortDNSLookupResults(tk.DNSDuplicateResponses)
-		tk.NetworkEvents = minipipeline.SortNetworkEvents(tk.NetworkEvents)
-		tk.TCPConnect = minipipeline.SortTCPConnectResults(tk.TCPConnect)
-		tk.TLSHandshakes = minipipeline.SortTLSHandshakeResults(tk.TLSHandshakes)
-	}
-
 	// return whether there was a fundamental failure, which would prevent
 	// the measurement from being submitted to the OONI collector.
 	return tk.fundamentalFailure
@@ -177,7 +159,3 @@ func registerExtensions(m *model.Measurement) {
 	model.ArchivalExtTLSHandshake.AddTo(m)
 	model.ArchivalExtTunnel.AddTo(m)
 }
-
-// SortObservations allows to configure sorting observations when that's needed to
-// reduce churn in the generated JSON files (mainly for minipipeline).
-var SortObservations = &atomic.Int64{}
diff --git a/internal/experiment/webconnectivitylte/qa_test.go b/internal/experiment/webconnectivitylte/qa_test.go
@@ -7,7 +7,6 @@ import (
 )
 
 func TestQA(t *testing.T) {
-	SortObservations.Add(1) // make sure we have predictable observations
 	for _, tc := range webconnectivityqa.AllTestCases() {
 		t.Run(tc.Name, func(t *testing.T) {
 			if (tc.Flags & webconnectivityqa.TestCaseFlagNoLTE) != 0 {

diff --git a/...inipipeline/testdata/webconnectivity/generated/badSSLWithExpiredCertificate/analysis.json b/...inipipeline/testdata/webconnectivity/generated/badSSLWithExpiredCertificate/analysis.json
@@ -58,7 +58,7 @@
       "Failure": "ssl_invalid_certificate",
       "TransactionID": 3,
       "TagFetchBody": true,
-      "DNSTransactionID": 2,
+      "DNSTransactionID": 1,
       "DNSDomain": "expired.badssl.com",
       "DNSLookupFailure": "",
       "DNSQueryType": null,
@@ -108,8 +108,8 @@
       "DNSTransactionID": 2,
       "DNSDomain": "expired.badssl.com",
       "DNSLookupFailure": "",
-      "DNSQueryType": "ANY",
-      "DNSEngine": "getaddrinfo",
+      "DNSQueryType": "A",
+      "DNSEngine": "udp",
       "DNSResolvedAddrs": [
         "104.154.89.105"
       ],
@@ -155,8 +155,8 @@
       "DNSTransactionID": 1,
       "DNSDomain": "expired.badssl.com",
       "DNSLookupFailure": "",
-      "DNSQueryType": "A",
-      "DNSEngine": "udp",
+      "DNSQueryType": "ANY",
+      "DNSEngine": "getaddrinfo",
       "DNSResolvedAddrs": [
         "104.154.89.105"
       ],
@@ -197,9 +197,9 @@
       "TagDepth": 0,
       "Type": 0,
       "Failure": "dns_no_answer",
-      "TransactionID": 1,
+      "TransactionID": 2,
       "TagFetchBody": null,
-      "DNSTransactionID": 1,
+      "DNSTransactionID": 2,
       "DNSDomain": "expired.badssl.com",
       "DNSLookupFailure": "dns_no_answer",
       "DNSQueryType": "AAAA",

diff --git a/...ine/testdata/webconnectivity/generated/badSSLWithExpiredCertificate/analysis_classic.json b/...ine/testdata/webconnectivity/generated/badSSLWithExpiredCertificate/analysis_classic.json
@@ -6,16 +6,16 @@
     "FinalResponseFailure": "unknown_error"
   },
   "DNSLookupSuccess": [
-    2
+    1
   ],
   "DNSLookupSuccessWithInvalidAddresses": [],
   "DNSLookupSuccessWithValidAddress": [
-    2
+    1
   ],
   "DNSLookupSuccessWithBogonAddresses": [],
   "DNSLookupSuccessWithInvalidAddressesClassic": [],
   "DNSLookupSuccessWithValidAddressClassic": [
-    2
+    1
   ],
   "DNSLookupUnexpectedFailure": [],
   "DNSLookupUnexplainedFailure": [],
@@ -55,7 +55,7 @@
       "Failure": "ssl_invalid_certificate",
       "TransactionID": 3,
       "TagFetchBody": true,
-      "DNSTransactionID": 2,
+      "DNSTransactionID": 1,
       "DNSDomain": "expired.badssl.com",
       "DNSLookupFailure": "",
       "DNSQueryType": null,
@@ -100,9 +100,9 @@
       "TagDepth": 0,
       "Type": 0,
       "Failure": "",
-      "TransactionID": 2,
+      "TransactionID": 1,
       "TagFetchBody": null,
-      "DNSTransactionID": 2,
+      "DNSTransactionID": 1,
       "DNSDomain": "expired.badssl.com",
       "DNSLookupFailure": "",
       "DNSQueryType": "ANY",