Addmoretests #3
Addmoretests #3
Conversation
# Conflicts: # internal/scti/handlers_test.go # Conflicts: # internal/scti/handlers_test.go
# Conflicts: # internal/scti/handlers_test.go
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @phbnf, I'm Gemini Code Assist1! 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 focuses on adding more tests to the internal/scti/handlers_test.go file, enhancing the test coverage for the AddChain and AddPreChain handlers. The changes include setting up a temporary directory for storage during tests, creating subdirectories for logs and issuers, and verifying the content of the leaf bundle after adding a chain or pre-chain. Additionally, there are minor updates to the internal/scti/handlers.go file, such as adding TODO comments for future improvements.
Highlights
- Test Coverage: Extensive tests added for
AddChainandAddPreChainhandlers, including checks for successful additions, duplicates, and chains without root certificates. - Storage Setup: Introduced temporary directories for test storage, ensuring a clean testing environment and preventing conflicts.
- Leaf Bundle Verification: Added verification steps to confirm the content of the leaf bundle after adding a chain or pre-chain, ensuring data integrity.
Changelog
Click here to see the changelog
- internal/scti/handlers.go
- Added TODO comments to
verifyAddChainfunction, suggesting the addition of tests and moving the function tochain_validation.go. - Added TODO comments to
entryFromChainfunction, suggesting moving it toct.goand adding tests.
- Added TODO comments to
- internal/scti/handlers_test.go
- Added imports for
crypto/x509,os,path,github.com/transparency-dev/trillian-tessera/api/layout, andgithub.com/transparency-dev/trillian-tessera/ctonly. - Introduced
logDirandissDirconstants for POSIX subdirectories. - Modified
setupTestLogfunction to create a temporary directory for storage. - Modified
newPosixStorageFuncto use the temporary directory for creating POSIX Tessera storage driver, issuer storage, and BBolt deduplication database. - Added
parseChainfunction to parse a chain of certificates and create actonly.Entry. - Enhanced
TestAddChainandTestAddPreChainto verify the content of the leaf bundle after adding a chain or pre-chain.
- Added imports for
- internal/types/staticct/staticct.go
- Added
Entrystruct to represent a CT log entry, including fields for timestamp, precert status, certificate data, issuer key hash, fingerprints, extensions, and leaf index. - Implemented
UnmarshalTextmethod for theEntrystruct to decode entry bundles from the Static CT API spec.
- Added
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 code we trust, tests we must,
To verify each handler's gust.
Chains and trees,
Put minds at ease,
Ensuring logs are free from rust.
Footnotes
-
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 review |
There was a problem hiding this comment.
Code Review
The pull request adds more tests to the scti package, focusing on handlers and data types. The changes include adding new test cases for AddChain and AddPreChain handlers, improving the newPosixStorageFunc to handle temporary directories, and introducing a new Entry type with UnmarshalText method for parsing CT log entries. Overall, the changes seem to improve the test coverage and data handling within the scti package.
Summary of Findings
- Missing tests for
verifyAddChain: TheverifyAddChainfunction lacks dedicated tests, as indicated by theTODOcomment. Adding tests for this function is crucial to ensure its correctness and reliability. - Potential for code duplication: The
entryFromChainfunction might be better placed inct.goto avoid potential code duplication and improve code organization. - Incomplete checks in
TestAddChainandTestAddPreChain: The tests forAddChainandAddPreChainare missing checks for the issuer chain fingerprint and inclusion proof. Implementing these checks would enhance the test coverage and ensure the integrity of the log entries. - TODO comments: There are a number of TODO comments that should be addressed to improve the code quality.
Merge Readiness
The pull request introduces valuable tests and data handling improvements. However, addressing the missing tests for verifyAddChain, considering the relocation of entryFromChain, and completing the checks in TestAddChain and TestAddPreChain would significantly enhance the quality and reliability of the code. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Given the medium and high severity issues identified, I recommend that the pull request not be merged until these are addressed.
| t.Errorf("Logged entry mismatch (-want +got):\n%s", diff) | ||
| } | ||
| // TODO(phbnf): check the issuer chain fingerprint | ||
| // TODO(phbnf): check inclusion proof | ||
| // TODO(phboneff): add a test with a backend write failure |
There was a problem hiding this comment.
Consider adding a test with a backend write failure to ensure the system handles such failures gracefully. This could involve simulating a disk full scenario or a permission error.
| // TODO(phbnf): add tests | ||
| // TODO(phbnf): move to chain_validation.go |
There was a problem hiding this comment.
It's great that you've added TODOs for tests and moving this function. Consider adding a brief description of what these tests should cover, or what the motivation is for moving the function.
| // TODO(phboneff): move to ct.go | ||
| // TODO(phboneff): add tests |
There was a problem hiding this comment.
Consider adding a brief description of what these tests should cover, or what the motivation is for moving the function.
| // TODO(phbnf): check the issuer chain fingerprint | ||
| // TODO(phbnf): check inclusion proof |
There was a problem hiding this comment.
These TODOs are great. It would be helpful to add a brief description of what these checks should entail.
| // TODO(phbnf): check the issuer chain fingerprint | ||
| // TODO(phbnf): check inclusion proof |
There was a problem hiding this comment.
These TODOs are great. It would be helpful to add a brief description of what these checks should entail.
| // TODO(phbnf): check the issuer chain fingerprint | ||
| // TODO(phbnf): check inclusion proof |
There was a problem hiding this comment.
These TODOs are great. It would be helpful to add a brief description of what these checks should entail.
| // TODO(phbnf): check the issuer chain fingerprint | ||
| // TODO(phbnf): check inclusion proof |
There was a problem hiding this comment.
These TODOs are great. It would be helpful to add a brief description of what these checks should entail.
There was a problem hiding this comment.
Code Review
The pull request adds more tests to the scti package, focusing on handlers and staticct types. The changes include adding new test cases for AddChain and AddPreChain handlers, as well as refactoring some test setup code for better reusability. The addition of tests for reading leaf bundles and comparing entries is a significant improvement. However, there are some areas where the code can be improved, particularly in terms of TODO comments and code clarity.
Summary of Findings
- Missing tests: The
verifyAddChainfunction lacks dedicated tests, and it's recommended to add them for better coverage. - Code placement: The
entryFromChainfunction could be moved toct.gofor better organization. - TODO comments: Several TODO comments exist in the code, indicating areas that need further attention, such as checking issuer chain fingerprints and inclusion proofs.
Merge Readiness
The pull request introduces valuable tests and improves the overall test coverage. However, the presence of TODO comments and the lack of tests for verifyAddChain indicate that there's still work to be done. I recommend addressing the TODO comments and adding tests for verifyAddChain before merging. I am unable to approve this pull request, and users should have others review and approve this code before merging.
| // TODO(phbnf): add tests | ||
| // TODO(phbnf): move to chain_validation.go |
There was a problem hiding this comment.
Consider adding tests for verifyAddChain to ensure it handles various scenarios correctly. Also, moving this function to chain_validation.go would improve code organization.
| // TODO(phboneff): move to ct.go | ||
| // TODO(phboneff): add tests |
There was a problem hiding this comment.
Moving entryFromChain to ct.go would align it with related functionality and improve modularity. Adding tests for this function is also recommended.
| // TODO(phbnf): check the issuer chain fingerprint | ||
| // TODO(phbnf): check inclusion proof |
There was a problem hiding this comment.
Consider adding checks for the issuer chain fingerprint and inclusion proof to ensure the integrity of the logged entry.
| // TODO(phbnf): check inclusion proof | ||
| // TODO(phbnf): add a test with a backend write failure |
There was a problem hiding this comment.
Adding a test case with a backend write failure would improve the robustness of the test suite.
| // TODO(phbnf): check the issuer chain fingerprint | ||
| // TODO(phbnf): check inclusion proof |
There was a problem hiding this comment.
Consider adding checks for the issuer chain fingerprint and inclusion proof to ensure the integrity of the logged entry.
| // TODO(phbnf): check inclusion proof | ||
| // TODO(phboneff): add a test with a backend write failure |
There was a problem hiding this comment.
Adding a test case with a backend write failure would improve the robustness of the test suite.
No description provided.