`handlers_test.go`: MOCK --> POSIX #231

phbnf · 2025-04-03T09:41:08Z

Towards #217, #5.

This PR reimplements handlers_test.go using a POSIX based log, rather than using a mock package.

Precisely, this PR:

introduces a POSIX issuer storage testonly implementation
refactors helper functions in handlers_test.go to initialise a log with a POSIX issuer storage, a POSIX Tessera backend, and a BBolt deduplication database
migrates all tests to this new test log implementation
Removes the mocking package and instructions
adds a GetRoots test
Removes a test where writes to Tessera fail. We'll need to implement this again in a followup PR. I couldn't do it in this PR because I could not find a straight forward way to generate a Tessera error from a test with a POSIX backend without making this PR too complex.

Followup PRs will add additional tests according to TODOs.

# Conflicts: # internal/scti/handlers_test.go

# Conflicts: # internal/scti/handlers_test.go # Conflicts: # internal/scti/handlers_test.go

# Conflicts: # internal/scti/handlers_test.go # Conflicts: # internal/scti/handlers_test.go # Conflicts: # internal/scti/handlers_test.go

# Conflicts: # internal/scti/handlers_test.go

AlCutter · 2025-04-03T10:10:52Z

internal/scti/handlers_test.go

+	server := setupTestServer(t, log, path.Join(prefix, "ct/v1/get-roots"))
+	defer server.Close()
+
+	t.Run("get-roots", func(t *testing.T) {


Why the t.Run?

woops, thanks

AlCutter · 2025-04-03T10:15:30Z

internal/scti/handlers_test.go

+
+func TestGetRoots(t *testing.T) {
+	log := setupTestLog(t)
+	server := setupTestServer(t, log, path.Join(prefix, "ct/v1/get-roots"))


Can you use types.GetRootsPath? (bunch of other places in the PR too for this and the other paths)

I can yeah - I did not do this intentionally because I see little benefits:

I find the end result less readable

this value is never going to change

AlCutter · 2025-04-03T10:17:52Z

internal/scti/handlers_test.go

-			recorder := makeAddPrechainRequest(t, info.handlers, chain)
-			if recorder.Code != test.want {
-				t.Fatalf("addPrechain()=%d (body:%v); want %d", recorder.Code, recorder.Body, test.want)
+			resp, err := http.Post(server.URL+"/ct/v1/add-pre-chain", "application/json", chain)


path.Join(server.URL, types.AddPreChainPath)?

Been there: I can't do this because server.URL contains http://, which path.Join does not understand, it will remove a forward slash.

How about url.JoinPath?

https://pkg.go.dev/net/url#JoinPath

AlCutter · 2025-04-03T10:19:17Z

internal/testonly/storage/posix/issuers.go

+func NewIssuerStorage(path string) (IssuersStorage, error) {
+	// Does nothing if the dictory already exists.
+	if err := os.MkdirAll(path, 0755); err != nil {
+		return "", fmt.Errorf("failed to create path %q: %w", path, err)


do you need to wrap the err so the caller can determine something and react accordingly, or is %v ok?

AlCutter · 2025-04-03T10:22:18Z

internal/testonly/storage/posix/issuers.go

+	if strings.Contains(string(key), "/") {
+		return "", fmt.Errorf("key %q cannot contain '/'", string(key))


Just flagging that this is potentially brittle for future uses if e.g. you decide to use this storage with base64 encoding for keys down the line.

Some filesystems might get upset with other unusual characters like \0, \\, or : too

Added a package docstring, and expended this function docstring to highlight that this might not work on all filesystems, and that it's certainly not for production use.

AlCutter · 2025-04-03T10:28:50Z

internal/testonly/storage/posix/issuers.go

+			} else {
+				return fmt.Errorf("failed to read object %q: %w", objName, err)
+			}


I think you don't need the indented else {} here since you'll either return or continue if errors.Is(os.ErrNotExist).

AlCutter · 2025-04-03T10:29:24Z

internal/testonly/storage/posix/issuers.go

+				return fmt.Errorf("failed to read object %q: %w", objName, err)
+			}
+		} else if bytes.Equal(f, kv.V) {
+			klog.V(2).Infof("AddIssuersIfNotExist: object %q already exists, continuing", objName)


nit:

Suggested change

klog.V(2).Infof("AddIssuersIfNotExist: object %q already exists, continuing", objName)

klog.V(2).Infof("AddIssuersIfNotExist: object %q already exists with identical contents, continuing", objName)

# Conflicts: # go.mod

roger2hk · 2025-04-04T10:45:21Z

internal/scti/handlers_test.go

+//   - a POSIX Tessera storage driver
+//   - a POSIX issuer storage system
+//   - a BBolt deduplication database
+func newPosixStorageFunc(t *testing.T) storage.CreateStorage {


Call this newPOSIXStorageFunc?

https://google.github.io/styleguide/go/decisions#initialisms

roger2hk · 2025-04-04T10:54:57Z

internal/scti/handlers_test.go

@@ -83,82 +79,129 @@ func (f *fixedTimeSource) Now() time.Time {
 	return f.fakeTime
 }

-// setupTest creates mock objects and contexts.  Caller should invoke info.mockCtrl.Finish().
-func setupTest(t *testing.T, pemRoots []string, signer crypto.Signer) handlerTestInfo {
+// setupTestLog creates test TesseraCT log using a POSIX backend.


Suggested change

// setupTestLog creates test TesseraCT log using a POSIX backend.

// setupTestLog creates a test TesseraCT log using the POSIX backend.

roger2hk · 2025-04-04T10:55:31Z

internal/scti/handlers_test.go

-		rejectExpired: false,
+	roots := x509util.NewPEMCertPool()
+	if err := roots.AppendCertsFromPEMFile(testRootPath); err != nil {
+		t.Fatalf("failed to read trusted roots: %v", err)


Suggested change

t.Fatalf("failed to read trusted roots: %v", err)

t.Fatalf("Failed to read trusted roots: %v", err)

roger2hk · 2025-04-04T13:50:44Z

internal/testonly/storage/posix/issuers.go

+	if strings.Contains(string(key), "/") {
+		return "", fmt.Errorf("key %q cannot contain '/'", string(key))


Suggested change

if strings.Contains(string(key), "/") {

return "", fmt.Errorf("key %q cannot contain '/'", string(key))

if strings.Contains(string(key), os.PathSeparator) {

return "", fmt.Errorf("key %q cannot contain '%s'", string(key), os.PathSeparator)

The / in test needs to be updated if this is updated.

https://pkg.go.dev/os#PathSeparator

phbnf added 7 commits April 3, 2025 09:25

posix issuer storage

174b01c

posix backend

e290664

# Conflicts: # internal/scti/handlers_test.go

test GetRoots

59784a1

# Conflicts: # internal/scti/handlers_test.go # Conflicts: # internal/scti/handlers_test.go

break down setup functions into two

7071688

# Conflicts: # internal/scti/handlers_test.go # Conflicts: # internal/scti/handlers_test.go # Conflicts: # internal/scti/handlers_test.go

NewPathHandlers test

42c751a

# Conflicts: # internal/scti/handlers_test.go

reintroduce get/post handler tests

8c6dfe7

# Conflicts: # internal/scti/handlers_test.go

addChain, addPreChain, addChainWhitespace tests

1740ce3

phbnf requested review from roger2hk and AlCutter April 3, 2025 09:41

phbnf assigned roger2hk and AlCutter Apr 3, 2025

phbnf force-pushed the posixtests branch from 031df30 to 2dde1f2 Compare April 3, 2025 09:49

add parseChain again to minimize diff

51c9282

phbnf force-pushed the posixtests branch from 2dde1f2 to 51c9282 Compare April 3, 2025 09:50

phbnf marked this pull request as ready for review April 3, 2025 09:50

phbnf marked this pull request as draft April 3, 2025 09:53

phbnf force-pushed the posixtests branch from a0eb3d2 to a64805e Compare April 3, 2025 09:57

phbnf marked this pull request as ready for review April 3, 2025 10:00

AlCutter approved these changes Apr 3, 2025

View reviewed changes

phbnf added 4 commits April 3, 2025 10:49

get rid of mocking

fe9fc0d

# Conflicts: # go.mod

clean var definition

a066d06

remove two TODO, tests are already in chain_validation_test.go

95017b4

address comments

87a6155

phbnf force-pushed the posixtests branch from a64805e to 87a6155 Compare April 3, 2025 10:50

AlCutter approved these changes Apr 3, 2025

View reviewed changes

roger2hk approved these changes Apr 4, 2025

View reviewed changes

address more comments

e84e25e

phbnf merged commit 0176f79 into transparency-dev:main Apr 4, 2025
7 checks passed

		if strings.Contains(string(key), "/") {
		return "", fmt.Errorf("key %q cannot contain '/'", string(key))

	klog.V(2).Infof("AddIssuersIfNotExist: object %q already exists, continuing", objName)
	klog.V(2).Infof("AddIssuersIfNotExist: object %q already exists with identical contents, continuing", objName)

	// setupTestLog creates test TesseraCT log using a POSIX backend.
	// setupTestLog creates a test TesseraCT log using the POSIX backend.

	t.Fatalf("failed to read trusted roots: %v", err)
	t.Fatalf("Failed to read trusted roots: %v", err)

handlers_test.go: MOCK --> POSIX #231

handlers_test.go: MOCK --> POSIX #231

Uh oh!

Conversation

phbnf commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phbnf Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

`handlers_test.go`: MOCK --> POSIX #231

`handlers_test.go`: MOCK --> POSIX #231

phbnf commented Apr 3, 2025 •

edited

Loading

phbnf Apr 3, 2025 •

edited

Loading