- Notifications
You must be signed in to change notification settings - Fork928
chore: executor_test: reduce test execution time#1876
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f118679 tob0260d7Comparecoderd/coderdtest/coderdtest.go Outdated
| SSHKeygenAlgorithm gitsshkey.Algorithm | ||
| APIRateLimit int | ||
| AutobuildTicker <-chan time.Time | ||
| AutobuildStatsChannel chan<- executor.RunStats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we could call thisAutobuildStats. The fact that it's a channel doesn't seem very important to the caller in this context.
| // WithStatsChannel will cause Executor to push a RunStats to ch after | ||
| // every tick. | ||
| func (e *Executor) WithStatsChannel(ch chan<- RunStats) *Executor { | ||
| e.statsCh = ch | ||
| return e | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we always do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
eg. maybe we could remove this and just make it the default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't have any major use-case for consuming these outside of the unit tests, hence the toggle.
Thoughts? Just pulling those values from the channel and dumping them to an underscore seems like a bit of a waste IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair enough. I think it's fine for now!
| // RunStats contains information about one run of Executor. | ||
| type RunStats struct { | ||
| Transitions map[uuid.UUID]database.WorkspaceTransition | ||
| Elapsed time.Duration | ||
| Error error | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could probably call thisStats. Sinceexecutor.Executor is the primary export of this package, it seems reasonableexecutor.Stats would attach to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just one question but otherwise LGTM!
coderd/coderdtest/coderdtest.go Outdated
| @@ -92,6 +93,11 @@ func NewWithAPI(t *testing.T, options *Options) (*codersdk.Client, *coderd.API) | |||
| options.AutobuildTicker = ticker | |||
| t.Cleanup(func() { close(ticker) }) | |||
| } | |||
| if options.AutobuildStatsChannel != nil { | |||
| t.Cleanup(func() { | |||
| close(options.AutobuildStatsChannel) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wondering if this could result in a send on nil channel at some point? Or is theRun() function guaranteed to have exited/not tick further at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In a unit test, the ticks are sent manually. So, the only way a send could occur after the channel is closed is if you did something like the below without pulling from the stats channel.
func TestSomething(t *testing.T) { // setup goes here blah blah t.Cleanup(func() { tickCh <- time.Now() }) // stats := <- statsCh}Additionally, calls tot.Cleanup are executed in LIFO order, so I'd suspect that you might run intogoleak complaining instead.
Removes 5-second wait in autobuild.executor unit tests:- Adds a write-only channel to Executor and plumbs through to unit tests- Modifies runOnce to return an executor.RunStats struct and write to statsCh if not nil
Uh oh!
There was an error while loading.Please reload this page.
This PR removes 5-second wait in
autobuild.executorunit tests (now approx. 1.6s on my machine):runOnceto return anexecutor.RunStatsstruct and write tostatsChif not nilCloses#1859