Remove filesystem and timer dependencies from container_runner_test.go #557
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
JoshuaKrstic
force-pushed
the
runnertestfix
branch
from
a544a24 to
4a2d7c2
Compare
JoshuaKrstic
force-pushed
the
runnertestfix
branch
2 times, most recently
from
2eccfae to
113c532
Compare
…test.go to reduce flakiness and runtime
JoshuaKrstic
force-pushed
the
runnertestfix
branch
from
113c532 to
6f61cc3
Compare
|
/gcbrun |
| } | ||
|
|
||
| // Write writes the data to a tmp file before copying it over to the desired location. | ||
| func (t *FileWriter) Write(token []byte) error { |
There was a problem hiding this comment.
This write function is specific to write a token file, but the file name (data_writer) is very generic, might want to change the name
| logger, | ||
| serialConsole, | ||
| tokenWriter, | ||
| timer, |
There was a problem hiding this comment.
I feel like adding a timer in ContainerRunner just for the ease of test might be a little bit too much.
If we have to use a timer, maybe offloading the token writing step to the attestAgent might be better.
There was a problem hiding this comment.
What do you mean by adding a timer? Are you saying adding an interface feels like too much?
alexmwu
left a comment
alexmwu
left a comment
There was a problem hiding this comment.
"reduce flakiness" can you explain the flakiness?
In general, I agree that this is quite complex just for enabling a test. We should at least get ride of the FS changes.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| tokenWriter, err := models.NewFileWriter(launcherfile.HostTmpPath, launcherfile.AttestationVerifierTokenFilename) |
There was a problem hiding this comment.
This could just be a utility function instead of creating so many new types.
| ) | ||
|
|
||
| // DataWriter is an interface for writing opaque data to some destination. | ||
| type DataWriter interface { |
There was a problem hiding this comment.
This interface already exists: https://pkg.go.dev/io#Writer.
| @@ -0,0 +1,46 @@ | |||
| // Package models contains models needed in client and server | |||
| package models | |||
There was a problem hiding this comment.
Why is this named model? It's only used in the client and it is also not very descriptive. https://google.github.io/styleguide/go/decisions.html#package-names
| runner := ContainerRunner{ | ||
| attestAgent: &fakeAttestationAgent{attestFunc: attestFunc}, | ||
| logger: logging.SimpleLogger(), | ||
| tokenWriter: tokenWriter, |
There was a problem hiding this comment.
I think the implementation could just be the function signature
fetchAndWriteTokenWithRetry(ctx context.Context,
retry func() *backoff.ExponentialBackOff, refresh func(ctx context.Context) (time.Duration, error)) error
That way you can just pass in a FakeRefresher with a Refresh and a GetNextToken method.
3 participants
This PR aims to remove time.Sleep() from container_runner_test.go to reduce flakiness and reduce test runtime.
To do this I added:
I put these in "models" to separate them from the core "container_runner" logic.
In addition, the test TestTokenIsNotChangedIfRefreshFails was removed as it is redundant with TestTokenRefreshRetryPolicyFail.