From 2f801bd1805c4993223c5fcc2dfea137a41eb32f Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Thu, 14 Sep 2023 10:17:50 +0200
Subject: [PATCH] refactor: rename geolocate enginelocate

I am soon going to start addressing the underlying issue described
by https://github.com/ooni/probe/issues/2531.

But, before doing that, I have noticed that the packets I need to
edit to this end are the following:

```
./internal/engine
./internal/geolocate
./internal/sessionresolver
```

Now, these three packages work in unison to provide an engine.Session
and they should sort together. However, I would like also to avoid
nesting because I think all of them servers an ~independent purpose
and geolocate and sessionresolver are possibly building blocks to
refactor or reimplement the engine.

For this reason, I have chosen to rename them such that it is clear
they are the engine and supporting packages.

This diff addresses the first half of the change by renaming the
geolocate package.

While there acknowledge that the script to rename packages was broken
and decide to ditch it for good rather than entering into the quest
of fixing it. I would probably have spent lots of time trying in doing
that, and my time is better spent otherwise. (I remember the official
package renaming tool did not support go modules, but probably it
does and I should look into giving it another spin.)
---
 internal/engine/experiment_test.go            | 20 +++++------
 internal/engine/session.go                    | 12 +++----
 internal/engine/session_internal_test.go      | 10 +++---
 .../{geolocate => enginelocate}/cloudflare.go |  2 +-
 .../cloudflare_test.go                        |  2 +-
 .../{geolocate => enginelocate}/fake_test.go  |  2 +-
 .../{geolocate => enginelocate}/geolocate.go  |  4 +--
 .../geolocate_test.go                         |  2 +-
 .../invalid_test.go                           |  2 +-
 .../{geolocate => enginelocate}/iplookup.go   |  2 +-
 .../iplookup_test.go                          |  2 +-
 .../{geolocate => enginelocate}/mmdblookup.go |  2 +-
 .../resolverlookup.go                         |  2 +-
 .../resolverlookup_test.go                    |  2 +-
 internal/{geolocate => enginelocate}/stun.go  |  2 +-
 .../{geolocate => enginelocate}/stun_test.go  |  2 +-
 .../{geolocate => enginelocate}/ubuntu.go     |  2 +-
 .../ubuntu_test.go                            |  2 +-
 pkg/oonimkall/session_integration_test.go     |  6 ++--
 script/gorenamepkg.bash                       | 36 -------------------
 20 files changed, 40 insertions(+), 76 deletions(-)
 rename internal/{geolocate => enginelocate}/cloudflare.go (97%)
 rename internal/{geolocate => enginelocate}/cloudflare_test.go (96%)
 rename internal/{geolocate => enginelocate}/fake_test.go (96%)
 rename internal/{geolocate => enginelocate}/geolocate.go (97%)
 rename internal/{geolocate => enginelocate}/geolocate_test.go (99%)
 rename internal/{geolocate => enginelocate}/invalid_test.go (92%)
 rename internal/{geolocate => enginelocate}/iplookup.go (99%)
 rename internal/{geolocate => enginelocate}/iplookup_test.go (98%)
 rename internal/{geolocate => enginelocate}/mmdblookup.go (92%)
 rename internal/{geolocate => enginelocate}/resolverlookup.go (97%)
 rename internal/{geolocate => enginelocate}/resolverlookup_test.go (97%)
 rename internal/{geolocate => enginelocate}/stun.go (99%)
 rename internal/{geolocate => enginelocate}/stun_test.go (99%)
 rename internal/{geolocate => enginelocate}/ubuntu.go (97%)
 rename internal/{geolocate => enginelocate}/ubuntu_test.go (98%)
 delete mode 100755 script/gorenamepkg.bash

diff --git a/internal/engine/experiment_test.go b/internal/engine/experiment_test.go
index 84298039fb..e8f5112d3b 100644
--- a/internal/engine/experiment_test.go
+++ b/internal/engine/experiment_test.go
@@ -3,12 +3,12 @@ package engine
 import (
 	"testing"
 
-	"github.com/ooni/probe-cli/v3/internal/geolocate"
+	"github.com/ooni/probe-cli/v3/internal/enginelocate"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
 func TestExperimentHonoursSharingDefaults(t *testing.T) {
-	measure := func(info *geolocate.Results) *model.Measurement {
+	measure := func(info *enginelocate.Results) *model.Measurement {
 		sess := &Session{location: info}
 		builder, err := sess.NewExperimentBuilder("example")
 		if err != nil {
@@ -19,48 +19,48 @@ func TestExperimentHonoursSharingDefaults(t *testing.T) {
 	}
 	type spec struct {
 		name         string
-		locationInfo *geolocate.Results
+		locationInfo *enginelocate.Results
 		expect       func(*model.Measurement) bool
 	}
 	allspecs := []spec{{
 		name:         "probeIP",
-		locationInfo: &geolocate.Results{ProbeIP: "8.8.8.8"},
+		locationInfo: &enginelocate.Results{ProbeIP: "8.8.8.8"},
 		expect: func(m *model.Measurement) bool {
 			return m.ProbeIP == model.DefaultProbeIP
 		},
 	}, {
 		name:         "probeASN",
-		locationInfo: &geolocate.Results{ASN: 30722},
+		locationInfo: &enginelocate.Results{ASN: 30722},
 		expect: func(m *model.Measurement) bool {
 			return m.ProbeASN == "AS30722"
 		},
 	}, {
 		name:         "probeCC",
-		locationInfo: &geolocate.Results{CountryCode: "IT"},
+		locationInfo: &enginelocate.Results{CountryCode: "IT"},
 		expect: func(m *model.Measurement) bool {
 			return m.ProbeCC == "IT"
 		},
 	}, {
 		name:         "probeNetworkName",
-		locationInfo: &geolocate.Results{NetworkName: "Vodafone Italia"},
+		locationInfo: &enginelocate.Results{NetworkName: "Vodafone Italia"},
 		expect: func(m *model.Measurement) bool {
 			return m.ProbeNetworkName == "Vodafone Italia"
 		},
 	}, {
 		name:         "resolverIP",
-		locationInfo: &geolocate.Results{ResolverIP: "9.9.9.9"},
+		locationInfo: &enginelocate.Results{ResolverIP: "9.9.9.9"},
 		expect: func(m *model.Measurement) bool {
 			return m.ResolverIP == "9.9.9.9"
 		},
 	}, {
 		name:         "resolverASN",
-		locationInfo: &geolocate.Results{ResolverASN: 44},
+		locationInfo: &enginelocate.Results{ResolverASN: 44},
 		expect: func(m *model.Measurement) bool {
 			return m.ResolverASN == "AS44"
 		},
 	}, {
 		name:         "resolverNetworkName",
-		locationInfo: &geolocate.Results{ResolverNetworkName: "Google LLC"},
+		locationInfo: &enginelocate.Results{ResolverNetworkName: "Google LLC"},
 		expect: func(m *model.Measurement) bool {
 			return m.ResolverNetworkName == "Google LLC"
 		},
diff --git a/internal/engine/session.go b/internal/engine/session.go
index fb988b31b0..423369c5a0 100644
--- a/internal/engine/session.go
+++ b/internal/engine/session.go
@@ -13,7 +13,7 @@ import (
 
 	"github.com/ooni/probe-cli/v3/internal/bytecounter"
 	"github.com/ooni/probe-cli/v3/internal/checkincache"
-	"github.com/ooni/probe-cli/v3/internal/geolocate"
+	"github.com/ooni/probe-cli/v3/internal/enginelocate"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
@@ -60,7 +60,7 @@ type Session struct {
 	byteCounter              *bytecounter.Counter
 	httpDefaultTransport     model.HTTPTransport
 	kvStore                  model.KeyValueStore
-	location                 *geolocate.Results
+	location                 *enginelocate.Results
 	logger                   model.Logger
 	proxyURL                 *url.URL
 	queryProbeServicesCount  *atomic.Int64
@@ -79,7 +79,7 @@ type Session struct {
 
 	// testLookupLocationContext is a an optional hook for testing
 	// allowing us to mock LookupLocationContext.
-	testLookupLocationContext func(ctx context.Context) (*geolocate.Results, error)
+	testLookupLocationContext func(ctx context.Context) (*enginelocate.Results, error)
 
 	// testMaybeLookupBackendsContext is an optional hook for testing
 	// allowing us to mock MaybeLookupBackendsContext.
@@ -676,8 +676,8 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {
 
 // LookupLocationContext performs a location lookup. If you want memoisation
 // of the results, you should use MaybeLookupLocationContext.
-func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results, error) {
-	task := geolocate.NewTask(geolocate.Config{
+func (s *Session) LookupLocationContext(ctx context.Context) (*enginelocate.Results, error) {
+	task := enginelocate.NewTask(enginelocate.Config{
 		Logger:    s.Logger(),
 		Resolver:  s.resolver,
 		UserAgent: s.UserAgent(),
@@ -687,7 +687,7 @@ func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results
 
 // lookupLocationContext calls testLookupLocationContext if set and
 // otherwise calls LookupLocationContext.
-func (s *Session) lookupLocationContext(ctx context.Context) (*geolocate.Results, error) {
+func (s *Session) lookupLocationContext(ctx context.Context) (*enginelocate.Results, error) {
 	if s.testLookupLocationContext != nil {
 		return s.testLookupLocationContext(ctx)
 	}
diff --git a/internal/engine/session_internal_test.go b/internal/engine/session_internal_test.go
index 6361484f31..454ccd5219 100644
--- a/internal/engine/session_internal_test.go
+++ b/internal/engine/session_internal_test.go
@@ -11,9 +11,9 @@ import (
 	"github.com/apex/log"
 	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/checkincache"
+	"github.com/ooni/probe-cli/v3/internal/enginelocate"
 	"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/geolocate"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/registry"
@@ -85,7 +85,7 @@ func TestSessionCheckInSuccessful(t *testing.T) {
 		},
 	}
 	s := &Session{
-		location: &geolocate.Results{
+		location: &enginelocate.Results{
 			ASN:         137,
 			CountryCode: "IT",
 		},
@@ -136,7 +136,7 @@ func TestSessionCheckInNetworkError(t *testing.T) {
 		Error: expect,
 	}
 	s := &Session{
-		location: &geolocate.Results{
+		location: &enginelocate.Results{
 			ASN:         137,
 			CountryCode: "IT",
 		},
@@ -178,7 +178,7 @@ func TestSessionCheckInCannotLookupLocation(t *testing.T) {
 func TestSessionCheckInCannotCreateProbeServicesClient(t *testing.T) {
 	errMocked := errors.New("mocked error")
 	s := &Session{
-		location: &geolocate.Results{
+		location: &enginelocate.Results{
 			ASN:         137,
 			CountryCode: "IT",
 		},
@@ -240,7 +240,7 @@ func TestSessionNewSubmitterWithCancelledContext(t *testing.T) {
 func TestSessionMaybeLookupLocationContextLookupLocationContextFailure(t *testing.T) {
 	errMocked := errors.New("mocked error")
 	sess := newSessionForTestingNoLookups(t)
-	sess.testLookupLocationContext = func(ctx context.Context) (*geolocate.Results, error) {
+	sess.testLookupLocationContext = func(ctx context.Context) (*enginelocate.Results, error) {
 		return nil, errMocked
 	}
 	err := sess.MaybeLookupLocationContext(context.Background())
diff --git a/internal/geolocate/cloudflare.go b/internal/enginelocate/cloudflare.go
similarity index 97%
rename from internal/geolocate/cloudflare.go
rename to internal/enginelocate/cloudflare.go
index 3de61a538b..17a69112a1 100644
--- a/internal/geolocate/cloudflare.go
+++ b/internal/enginelocate/cloudflare.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/cloudflare_test.go b/internal/enginelocate/cloudflare_test.go
similarity index 96%
rename from internal/geolocate/cloudflare_test.go
rename to internal/enginelocate/cloudflare_test.go
index 4f0b44da41..c3213c5060 100644
--- a/internal/geolocate/cloudflare_test.go
+++ b/internal/enginelocate/cloudflare_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/fake_test.go b/internal/enginelocate/fake_test.go
similarity index 96%
rename from internal/geolocate/fake_test.go
rename to internal/enginelocate/fake_test.go
index 5dc683f70b..72933e9ab9 100644
--- a/internal/geolocate/fake_test.go
+++ b/internal/enginelocate/fake_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"net/http"
diff --git a/internal/geolocate/geolocate.go b/internal/enginelocate/geolocate.go
similarity index 97%
rename from internal/geolocate/geolocate.go
rename to internal/enginelocate/geolocate.go
index a9c6f5f23d..ce3d282667 100644
--- a/internal/geolocate/geolocate.go
+++ b/internal/enginelocate/geolocate.go
@@ -1,5 +1,5 @@
-// Package geolocate implements IP lookup, resolver lookup, and geolocation.
-package geolocate
+// Package enginelocate implements IP lookup, resolver lookup, and geolocation.
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/geolocate_test.go b/internal/enginelocate/geolocate_test.go
similarity index 99%
rename from internal/geolocate/geolocate_test.go
rename to internal/enginelocate/geolocate_test.go
index 9d10917531..7a69a68fd1 100644
--- a/internal/geolocate/geolocate_test.go
+++ b/internal/enginelocate/geolocate_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/invalid_test.go b/internal/enginelocate/invalid_test.go
similarity index 92%
rename from internal/geolocate/invalid_test.go
rename to internal/enginelocate/invalid_test.go
index bb62404328..a6ac6b7f97 100644
--- a/internal/geolocate/invalid_test.go
+++ b/internal/enginelocate/invalid_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/iplookup.go b/internal/enginelocate/iplookup.go
similarity index 99%
rename from internal/geolocate/iplookup.go
rename to internal/enginelocate/iplookup.go
index 3d9711f323..e82adb1415 100644
--- a/internal/geolocate/iplookup.go
+++ b/internal/enginelocate/iplookup.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/iplookup_test.go b/internal/enginelocate/iplookup_test.go
similarity index 98%
rename from internal/geolocate/iplookup_test.go
rename to internal/enginelocate/iplookup_test.go
index 2739a95305..e3c11d6935 100644
--- a/internal/geolocate/iplookup_test.go
+++ b/internal/enginelocate/iplookup_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/mmdblookup.go b/internal/enginelocate/mmdblookup.go
similarity index 92%
rename from internal/geolocate/mmdblookup.go
rename to internal/enginelocate/mmdblookup.go
index 14e901f3fa..25af99d9dd 100644
--- a/internal/geolocate/mmdblookup.go
+++ b/internal/enginelocate/mmdblookup.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"github.com/ooni/probe-cli/v3/internal/geoipx"
diff --git a/internal/geolocate/resolverlookup.go b/internal/enginelocate/resolverlookup.go
similarity index 97%
rename from internal/geolocate/resolverlookup.go
rename to internal/enginelocate/resolverlookup.go
index 85da4dee3d..fe93b2a21e 100644
--- a/internal/geolocate/resolverlookup.go
+++ b/internal/enginelocate/resolverlookup.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/resolverlookup_test.go b/internal/enginelocate/resolverlookup_test.go
similarity index 97%
rename from internal/geolocate/resolverlookup_test.go
rename to internal/enginelocate/resolverlookup_test.go
index 8d741e7101..c02110a221 100644
--- a/internal/geolocate/resolverlookup_test.go
+++ b/internal/enginelocate/resolverlookup_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/stun.go b/internal/enginelocate/stun.go
similarity index 99%
rename from internal/geolocate/stun.go
rename to internal/enginelocate/stun.go
index d6133a1096..b0600b65e5 100644
--- a/internal/geolocate/stun.go
+++ b/internal/enginelocate/stun.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/stun_test.go b/internal/enginelocate/stun_test.go
similarity index 99%
rename from internal/geolocate/stun_test.go
rename to internal/enginelocate/stun_test.go
index d8c571fc27..5efb96207b 100644
--- a/internal/geolocate/stun_test.go
+++ b/internal/enginelocate/stun_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/ubuntu.go b/internal/enginelocate/ubuntu.go
similarity index 97%
rename from internal/geolocate/ubuntu.go
rename to internal/enginelocate/ubuntu.go
index 0f75afb4e0..de59ec9d89 100644
--- a/internal/geolocate/ubuntu.go
+++ b/internal/enginelocate/ubuntu.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/internal/geolocate/ubuntu_test.go b/internal/enginelocate/ubuntu_test.go
similarity index 98%
rename from internal/geolocate/ubuntu_test.go
rename to internal/enginelocate/ubuntu_test.go
index d6edf7c015..e385b1b0f2 100644
--- a/internal/geolocate/ubuntu_test.go
+++ b/internal/enginelocate/ubuntu_test.go
@@ -1,4 +1,4 @@
-package geolocate
+package enginelocate
 
 import (
 	"context"
diff --git a/pkg/oonimkall/session_integration_test.go b/pkg/oonimkall/session_integration_test.go
index a86eac412f..ab1bf45b30 100644
--- a/pkg/oonimkall/session_integration_test.go
+++ b/pkg/oonimkall/session_integration_test.go
@@ -11,7 +11,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/ooni/probe-cli/v3/internal/geolocate"
+	"github.com/ooni/probe-cli/v3/internal/enginelocate"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/pkg/oonimkall"
 )
@@ -50,7 +50,7 @@ func ReduceErrorForGeolocate(err error) error {
 	if errors.Is(err, context.Canceled) {
 		return nil // when we have not downloaded the resources yet
 	}
-	if !errors.Is(err, geolocate.ErrAllIPLookuppersFailed) {
+	if !errors.Is(err, enginelocate.ErrAllIPLookuppersFailed) {
 		return nil // otherwise
 	}
 	return fmt.Errorf("not the error we expected: %w", err)
@@ -295,7 +295,7 @@ func TestCheckInLookupLocationFailure(t *testing.T) {
 	config.WebConnectivity.AddCategory("CULTR")
 	ctx.Cancel() // immediate failure
 	result, err := sess.CheckIn(ctx, &config)
-	if !errors.Is(err, geolocate.ErrAllIPLookuppersFailed) {
+	if !errors.Is(err, enginelocate.ErrAllIPLookuppersFailed) {
 		t.Fatalf("not the error we expected: %+v", err)
 	}
 	if result != nil {
diff --git a/script/gorenamepkg.bash b/script/gorenamepkg.bash
deleted file mode 100755
index 3dc476ed84..0000000000
--- a/script/gorenamepkg.bash
+++ /dev/null
@@ -1,36 +0,0 @@
-#!/bin/bash
-set -euo pipefail
-
-if [[ $# -ne 2 ]]; then
-	echo "usage: $0 {name-before} {name-after}" 1>&2
-	exit 1
-fi
-name_before=$1
-shift
-name_after=$1
-shift
-
-basename_before=$(basename $name_before)
-basename_after=$(basename $name_after)
-
-echo "git mv $name_before $name_after"
-git mv $name_before $name_after
-
-for file in $(find $name_after -type f -name \*.go); do
-	echo "replacing the package name of $file"
-	cat $file | sed -e "s|^package $basename_before|package $basename_after|g" \
-		-e "s|^// Package $basename_before|// Package $basename_after|g" > $file.tmp
-	cat $file.tmp > $file
-	rm $file.tmp
-done
-
-pkg_prefix=github.com/ooni/probe-cli/v3
-pkg_before=$pkg_prefix/$name_before
-pkg_after=$pkg_prefix/$name_after
-
-for file in $(find . -type f -name \*.go); do
-	echo "editing the import path of $file"
-	cat $file | sed -e "s|\"$pkg_before|\"$pkg_after|g" > $file.tmp
-	cat $file.tmp > $file
-	rm $file.tmp
-done