Commit 01550a3
Build root command from factory (#440)
Instantiates the Cobra singleton `rootCmd` from a function that builds it out, instead of directly building the singleton inside of the module.
---
Pulled this out of a too-big PR for [GROW-2118: PG Create](https://linear.app/render-com/issue/GROW-2118/pg-create).
I want to be able to instantiate the application inside of a test, instead of executing commands against the rootCmd singleton.
When we execute commands against the rootCmd singleton, it holds on to global state that the real production application would never hold on to, such as the flags set.
This makes it easier to write tests _and_ better simulates production!
The singleton works fine in production because a CLI process executes once, but in tests we run many command executions in the same Go process. Cobra command objects hold mutable state, including parsed flag values, changed flags, output writers, args, and context. That means tests can accidentally share state that the production CLI would never share.
There are a couple levels of testing we can choose from. The smallest unit test is to call a command’s `RunE` directly:
```go
cmd := newPgCreateCmd(deps)
cmd.RunE(cmd, nil)
```
That can be useful for narrow command behavior, but it skips some of the CLI surface a user actually exercises: command routing, argument parsing, persistent flags, persistent hooks, and dependency setup.
For higher-fidelity tests, I want to pass args into the CLI and let Cobra execute the command:
Before:
```go
rootCmd.SetArgs([]string{"ea", "pg", "create", "--output", "json"})
err := rootCmd.Execute()
```
That reuses the global command tree and has to be careful about resetting flags and other state.
Today, that often means tests need cleanup like this between executions:
```go
rootCmd.SetArgs(nil)
rootCmd.PersistentFlags().VisitAll(func(f *pflag.Flag) {
f.Changed = false
f.Value.Set(f.DefValue)
})
pgCreateCmd.Flags().VisitAll(func(f *pflag.Flag) {
f.Changed = false
f.Value.Set(f.DefValue)
})
```
An example of state persisting between executions:
```go
// Test A
rootCmd.SetArgs([]string{"ea", "pg", "create", "--output", "json"})
_ = rootCmd.Execute()
// Test B
rootCmd.SetArgs([]string{"ea", "pg", "create"})
_ = rootCmd.Execute()
```
If both tests reuse the same Cobra command objects, Test B can accidentally observe state mutated by Test A, such as a flag still being marked as changed. A real user running `render ea pg create` would get a fresh process and a fresh command tree.
After:
```go
// In the ideal state, we would just need to do: app = newApplication
// Today, because we have a mix of ways that commands get attached to rootCmd,
// Tests need to build out their own command tree
// which does lower the fidelity a bit
root := newRootCmd()
setupPGCommands(root, deps)
setupRootCmdPersistentRun(root, deps)
root.SetArgs([]string{"ea", "pg", "create", "--output", "json"})
err := root.Execute()
```
Each test can build a fresh command tree, closer to how a real CLI invocation starts from a clean process.
There is still a mix of command construction styles today. Some commands are already factories:
```go
cmd := NewRunListCmd(deps)
cmd := NewLogsCmd(deps)
cmd := newLogoutCmd()
```
Many others are still package-level singletons:
```go
var kvCreateCmd = &cobra.Command{...}
var EarlyAccessCmd = &cobra.Command{...}
var servicesCmd = &cobra.Command{...}
```
Moving the root command to a factory does not require converting every command all at once. It gives us a better place to opt individual command subtrees into the factory pattern when we want the testing benefits. If this direction feels good, I can keep migrating commands over time instead of doing one giant refactor.
The end-state could be even cleaner: once the commands are factory-built, tests should not need to know about `setupPGCommands` or any other command-specific setup function. We could have something like:
```go
root := newApplication(deps)
root.SetArgs([]string{"ea", "pg", "create", "--output", "json"})
err := root.Execute()
```
`newApplication()` would build the root command, attach all commands, wire dependencies, and return the full CLI app ready to execute. That gets us the production-shaped test harness without forcing every test to hand-pick command subtrees.
One nice side effect: this should also make `t.Parallel()` possible for command tests. If every test gets its own command tree, tests are no longer racing over shared Cobra state like args, flag values, output writers, and command context. We still need each test to isolate other process-level things like env vars, config paths, and fake API servers, but this removes one of the biggest shared-state blockers.
GitOrigin-RevId: d1ee179e985c936a15e8ae737ad4b2593e9bcefe1 parent 616ce1d commit 01550a3
1 file changed
Lines changed: 60 additions & 60 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
39 | 41 | | |
40 | 42 | | |
41 | 43 | | |
42 | 44 | | |
43 | 45 | | |
44 | | - | |
45 | | - | |
46 | | - | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
47 | 49 | | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | | - | |
53 | | - | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
54 | 56 | | |
55 | | - | |
| 57 | + | |
56 | 58 | | |
57 | | - | |
| 59 | + | |
58 | 60 | | |
59 | | - | |
60 | | - | |
61 | | - | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
62 | 65 | | |
63 | | - | |
64 | 66 | | |
65 | | - | |
66 | | - | |
67 | | - | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
68 | 70 | | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
73 | 77 | | |
| 78 | + | |
74 | 79 | | |
75 | | - | |
| 80 | + | |
| 81 | + | |
76 | 82 | | |
77 | | - | |
78 | | - | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
79 | 102 | | |
80 | 103 | | |
81 | 104 | | |
| |||
149 | 172 | | |
150 | 173 | | |
151 | 174 | | |
152 | | - | |
| 175 | + | |
153 | 176 | | |
154 | 177 | | |
155 | 178 | | |
156 | 179 | | |
157 | | - | |
158 | | - | |
| 180 | + | |
| 181 | + | |
159 | 182 | | |
160 | 183 | | |
161 | 184 | | |
| |||
293 | 316 | | |
294 | 317 | | |
295 | 318 | | |
296 | | - | |
297 | | - | |
298 | | - | |
299 | | - | |
300 | | - | |
301 | | - | |
| 319 | + | |
| 320 | + | |
302 | 321 | | |
303 | 322 | | |
304 | 323 | | |
| |||
308 | 327 | | |
309 | 328 | | |
310 | 329 | | |
311 | | - | |
312 | | - | |
313 | | - | |
314 | | - | |
315 | | - | |
316 | | - | |
317 | | - | |
318 | | - | |
319 | | - | |
320 | | - | |
321 | | - | |
322 | | - | |
323 | | - | |
324 | | - | |
325 | | - | |
326 | | - | |
327 | | - | |
328 | | - | |
329 | | - | |
330 | 330 | | |
331 | 331 | | |
332 | 332 | | |
| |||
0 commit comments