Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented and Tested GetRandom #277

Merged
merged 2 commits into from
May 26, 2022

Conversation

matt-tsai
Copy link
Contributor

No description provided.

@matt-tsai matt-tsai requested a review from a team as a code owner May 24, 2022 20:41
Copy link
Member

@chrisfenner chrisfenner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for the change! I have two tiny nits.

direct/tpm2/get_random_test.go Outdated Show resolved Hide resolved
direct/tpm2/tpm2.go Outdated Show resolved Hide resolved
direct/tpm2/tpm2.go Show resolved Hide resolved
@@ -0,0 +1,24 @@
package tpm2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not a blocking comment) how do we want to organize tests? having a test per command may make the directory quite large. we could have a test directory like in tpm2 or we could split by use case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mild opinion is that I like having more and smaller test files. A test directory with e.g., one file per command tested would be groovy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree this makes tests easier to find, and we should go for this. I do wonder about tests that use multiple commands like a potential policy auth test with multiple policies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, and we'd have a lot of helpers too. So we might have some per-command tests as well as some "integration-y" tests that really cover a bunch of commands. That's a good reason not to be rigid on "one file per command" but in general I think it's ok to have a lot of files.

@chrisfenner
Copy link
Member

LGTM - Matt can you update your branch with a rebase?

@chrisfenner chrisfenner merged commit d1d2d19 into google:tpmdirect May 26, 2022
@matt-tsai matt-tsai deleted the getRandomBranch branch May 26, 2022 18:43
matt-tsai added a commit to matt-tsai/go-tpm that referenced this pull request Jun 22, 2022
* Implement and tested go-tpm direct function TPM2_GetRandom

* Resolved all nits from PR

Co-authored-by: Matthew Tsai <matthewtsai@google.com>
@matt-tsai matt-tsai changed the title Implement and tested go-tpm direct function TPM2_GetRandom Implemented and Tested TPM2_GetRandom Jul 27, 2022
@matt-tsai matt-tsai changed the title Implemented and Tested TPM2_GetRandom Implemented and Tested GetRandom Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants