Posixtests #2

phbnf · 2025-04-03T08:29:14Z

No description provided.

gemini-code-assist

Hello @phbnf, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request introduces several changes to the internal/scti/handlers_test.go file, primarily focusing on refactoring the testing setup and adding a new test case for the get-roots endpoint. It replaces the previous mock-based testing approach with a more integrated approach using a POSIX backend for the TesseraCT log. Additionally, a new issuer storage implementation based on the POSIX filesystem is introduced in internal/testonly/storage/posix/issuers.go and internal/testonly/storage/posix/issuers_test.go. The changes also involve updating the test functions to use the new testing setup and removing some unnecessary code.

Highlights

Refactor Testing Setup: The pull request refactors the testing setup in internal/scti/handlers_test.go by replacing the mock-based approach with a more integrated approach using a POSIX backend for the TesseraCT log. This involves creating a setupTestLog function to create a test TesseraCT log using a POSIX backend, and a setupTestServer function to create a test server with a specified endpoint.
Add GetRoots Test Case: A new test case, TestGetRoots, is added to internal/scti/handlers_test.go to verify the functionality of the get-roots endpoint. This test case sets up a test server, makes a GET request to the endpoint, and verifies the response status code and content.
Introduce POSIX Issuer Storage: The pull request introduces a new issuer storage implementation based on the POSIX filesystem in internal/testonly/storage/posix/issuers.go and internal/testonly/storage/posix/issuers_test.go. This implementation provides a way to store and retrieve issuer certificates using the local filesystem.
Update Test Functions: The test functions in internal/scti/handlers_test.go are updated to use the new testing setup and the POSIX backend. This involves removing the mock-related code and updating the test assertions to reflect the new testing environment.
Remove Unnecessary Code: The pull request removes some unnecessary code from internal/scti/handlers_test.go, such as the handlerTestInfo struct, the fixedTimeSource struct, and some unused variables.

Changelog

Click here to see the changelog

internal/scti/handlers_test.go
- Refactors testing setup to use POSIX backend instead of mocks.
- Adds TestGetRoots test case for the get-roots endpoint.
- Updates test functions to use the new testing setup.
- Removes unnecessary code and variables.
- Removes dependency on github.com/golang/mock/gomock and google.golang.org/grpc/*
internal/testonly/storage/posix/issuers.go
- Introduces a new issuer storage implementation based on the POSIX filesystem.
- Implements the AddIssuersIfNotExist method to store issuer certificates.
internal/testonly/storage/posix/issuers_test.go
- Adds tests for the POSIX issuer storage implementation.
- Tests the NewIssuerStorage, keyToObjName, and AddIssuersIfNotExist methods.
internal/types/rfc6962.go
- Adds GetRootsResponse struct to represent the JSON response to the get-roots GET method.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

In halls of code, where tests reside,
Mocks fade, POSIX takes the stride.
Roots sought, servers rise,
A new test paradigm applies.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

The pull request introduces POSIX tests for the static-ct project, enhancing the testing infrastructure by incorporating a POSIX backend for TesseraCT log and issuer storage. The changes include refactoring existing test setups, adding new test functions, and integrating POSIX storage functionalities. Overall, the changes seem well-structured and contribute positively to the project's robustness.

Summary of Findings

Removal of mock objects: The removal of mock objects and the introduction of real POSIX-based storage for testing is a significant change. While it improves the fidelity of the tests, it's crucial to ensure that the POSIX storage is properly cleaned up and doesn't leave any lingering state that could affect other tests.
TODO comments: There are a few TODO comments in the code, specifically regarding additional options for tessera.NewAppendOptions() and a parseBodyJSONChain test. Addressing these would improve the completeness and maintainability of the code.
Test Coverage: While the changes introduce new tests, it's important to ensure that all critical code paths are covered, especially those related to error handling and edge cases in the POSIX storage implementation.

Merge Readiness

The pull request introduces significant improvements to the testing infrastructure by integrating POSIX-based storage. However, there are a few issues that should be addressed before merging. Specifically, the TODO comments should be resolved, and test coverage should be thoroughly reviewed to ensure all critical code paths are covered. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Given the medium severity comments, I recommend that the pull request not be merged until those are addressed (at a minimum).

gemini-code-assist · 2025-04-03T08:29:35Z

internal/scti/handlers_test.go

+			// TODO(phboneff): add other options like MaxBatchSize of 1 when implementing
+			// additional tests


Consider adding other options like MaxBatchSize of 1 when implementing additional tests.

Suggested change

// TODO(phboneff): add other options like MaxBatchSize of 1 when implementing

// additional tests

// TODO(phboneff): add other options like MaxBatchSize of 1 when implementing

// additional tests

# 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

phbnf · 2025-04-03T09:28:16Z

/gemini review

gemini-code-assist

Code Review

The pull request introduces several changes to the handlers_test.go file, including refactoring test setup, adding tests for get-roots, add-chain, and add-pre-chain endpoints, and updating dependencies. The new tests use a POSIX backend for the TesseraCT log. Overall, the changes seem to improve the test coverage and clarity of the scti package.

Summary of Findings

Missing error check: In TestAddChain and TestAddPreChain, the error returned by http.Post is not checked before accessing the resp.StatusCode. This could lead to a panic if the request fails and resp is nil.
Inconsistent path joining: In TestGetRoots, TestAddChainWhitespace, TestAddChain, and TestAddPreChain, the path is joined using path.Join and then concatenated with /ct/v1/.... This is redundant and can be simplified.
TODO comments: There are several TODO comments in TestAddChain and TestAddPreChain that indicate missing test cases or functionality. These should be addressed to ensure complete test coverage.
Removal of mock-based tests: The removal of mock-based tests and introduction of POSIX backend tests is a good step towards integration tests, but it's important to ensure that the new tests cover all the scenarios previously covered by the mock tests.

Merge Readiness

The pull request introduces significant improvements to the test suite. However, the missing error check in TestAddChain and TestAddPreChain is a critical issue that must be addressed before merging. Additionally, the TODO comments indicate incomplete test coverage, which should be improved before merging. I am unable to directly approve this pull request, and users should have others review and approve this code before merging. I recommend addressing the critical issue and the TODO comments before merging.

gemini-code-assist · 2025-04-03T09:28:34Z

internal/scti/handlers_test.go

-				t.Fatalf("%q path not registered", types.AddChainStr)
-			}
-			req, err := http.NewRequest(http.MethodPost, "http://example.com/ct/v1/add-chain", strings.NewReader(test.body))
+			resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", strings.NewReader(test.body))


The error returned by http.Post is not checked before accessing resp.StatusCode. This could lead to a panic if the request fails and resp is nil. It's important to handle potential errors to ensure the stability of the test.

Suggested change

resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", strings.NewReader(test.body))

resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", strings.NewReader(test.body))

if err != nil {

t.Fatalf("http.Post(%s)=(_,%q); want (_,nil)", types.AddChainPath, err)

}

gemini-code-assist · 2025-04-03T09:28:34Z

internal/scti/handlers_test.go

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


The error returned by http.Post is not checked before accessing resp.StatusCode. This could lead to a panic if the request fails and resp is nil. It's important to handle potential errors to ensure the stability of the test.

Suggested change

resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", chain)

resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", chain)

if err != nil {

t.Fatalf("http.Post(%s)=(_,%q); want (_,nil)", types.AddChainPath, err)

}

gemini-code-assist · 2025-04-03T09:28:34Z

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)


The error returned by http.Post is not checked before accessing resp.StatusCode. This could lead to a panic if the request fails and resp is nil. It's important to handle potential errors to ensure the stability of the test.

Suggested change

resp, err := http.Post(server.URL+"/ct/v1/add-pre-chain", "application/json", chain)

resp, err := http.Post(server.URL+"/ct/v1/add-pre-chain", "application/json", chain)

if err != nil {

t.Fatalf("http.Post(%s)=(_,%q); want (_,nil)", types.AddPreChainPath, err)

}

gemini-code-assist · 2025-04-03T09:28:34Z

internal/scti/handlers_test.go

-				t.Fatalf("%q path not registered", types.AddChainStr)
-			}
-			req, err := http.NewRequest(http.MethodPost, "http://example.com/ct/v1/add-chain", strings.NewReader(test.body))
+			resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", strings.NewReader(test.body))


The path is joined using path.Join and then concatenated with /ct/v1/.... This is redundant and can be simplified by directly using the server.URL which already contains the base path.

Suggested change

resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", strings.NewReader(test.body))

resp, err := http.Post(server.URL, "application/json", strings.NewReader(test.body))

gemini-code-assist · 2025-04-03T09:28:34Z

internal/scti/handlers_test.go

+				// TODO(phboneff): read from the log and compare values
+				// TODO(phboneff): add a test with a backend write failure
 				// TODO(phboneff): check that the index is in the SCT
 				// TODO(phboneff): add a test with a not after range
 				// TODO(phboneff): add a test with a start date only
+				// TODO(phboneff): add duplicate tests


These TODO comments indicate missing test cases or functionality. It's important to address these to ensure complete test coverage. Consider adding tests for backend write failures, SCT index verification, not-after range, start date only, and duplicate entries.

gemini-code-assist · 2025-04-03T09:28:34Z

internal/scti/handlers_test.go

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


The path is joined using path.Join and then concatenated with /ct/v1/.... This is redundant and can be simplified by directly using the server.URL which already contains the base path.

Suggested change

resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", chain)

resp, err := http.Post(server.URL, "application/json", chain)

gemini-code-assist · 2025-04-03T09:28:34Z

internal/scti/handlers_test.go

+				// TODO(phboneff): read from the log and compare values
+				// TODO(phboneff): add a test with a backend write failure
+				// TODO(phboneff): check that the index is in the SCT
+				// TODO(phboneff): add a test with a not after range
+				// TODO(phboneff): add a test with a start date only
+				// TODO(phboneff): add duplicate tests


These TODO comments indicate missing test cases or functionality. It's important to address these to ensure complete test coverage. Consider adding tests for backend write failures, SCT index verification, not-after range, start date only, and duplicate entries.

gemini-code-assist · 2025-04-03T09:28:34Z

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)


The path is joined using path.Join and then concatenated with /ct/v1/.... This is redundant and can be simplified by directly using the server.URL which already contains the base path.

Suggested change

resp, err := http.Post(server.URL+"/ct/v1/add-pre-chain", "application/json", chain)

resp, err := http.Post(server.URL, "application/json", chain)

# Conflicts: # go.mod

gemini-code-assist bot reviewed Apr 3, 2025

View reviewed changes

phbnf force-pushed the posixtests branch 5 times, most recently from 98f0e28 to 25d3ff2 Compare April 3, 2025 09:06

phbnf added 6 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

phbnf force-pushed the posixtests branch from 25d3ff2 to 9f71239 Compare April 3, 2025 09:26

gemini-code-assist bot suggested changes Apr 3, 2025

View reviewed changes

phbnf force-pushed the posixtests branch from 9f71239 to 2ab3c16 Compare April 3, 2025 09:34

addChain, addPreChain, addChainWhitespace tests

1740ce3

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

add parseChain again to minimize diff

51c9282

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

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

address more comments

e84e25e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Posixtests #2

Posixtests #2

Uh oh!

phbnf commented Apr 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

phbnf commented Apr 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

gemini-code-assist bot Apr 3, 2025

Uh oh!

Uh oh!

		// TODO(phboneff): add other options like MaxBatchSize of 1 when implementing
		// additional tests

	resp, err := http.Post(server.URL+"/ct/v1/add-chain", "application/json", strings.NewReader(test.body))
	resp, err := http.Post(server.URL, "application/json", strings.NewReader(test.body))

Posixtests #2

Are you sure you want to change the base?

Posixtests #2

Uh oh!

Conversation

phbnf commented Apr 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

phbnf commented Apr 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!