Fix run command exit codes #1733
Conversation
| @@ -245,7 +245,7 @@ export async function runAndCopy({ | |||
| result = await runner.exec({ | |||
| // Pipe the output from the command to the /tmp/output pipe, including stderr. Some shell voodoo happening | |||
There was a problem hiding this comment.
This comment still applies 😛
garden-service/src/commands/base.ts
Outdated
| actionDescription: string | ||
| }) { | ||
| const prefix = success | ||
| ? `${capitalize(actionDescription)} result:` |
There was a problem hiding this comment.
Wondering about the phrasing here. "Run task result" and "Run test result" feel a little odd to me. How about using "output" instead of "result", and just using "Task" and "Test" for the action description? So it'd be simply "Task output" and "Test output". That feels more natural to me.
There was a problem hiding this comment.
Which btw also works for the error variant, e.g. "Test failed with error".
docs/misc/faq.md
Outdated
|
|
||
| The `garden run test` command runs **a single test in interactive mode** regardless of whether or not it's cached. Interactive mode means that the output is streamed to the screen immediately and you can interact with it if applicable. | ||
|
|
||
| Note that due to a current limitation, Garden can't copy artifacts for tests in interactive mode. You can disable it by setting `--interactive false`. [See here](https://docs.garden.io/reference/commands#garden-run-test) for the full synopsis. |
There was a problem hiding this comment.
Is there an open issue for that? Good to link that here.
There was a problem hiding this comment.
No but we should add it.
| } | ||
|
|
||
| return { result } | ||
| return handleTaskResult({ log, actionDescription: "run task", result: result! }) |
There was a problem hiding this comment.
Should this be handleRunResult?
There was a problem hiding this comment.
It should not. This handleTaskResult function handles, as the name suggests, a result of type TaskResult, from a garden.processTasks call. The handleRunResult handles results of type RunResult, from calling something like garden.testModule.
I opted for two functions since the types, and especially return types, are different and instead enforcing correctness at the test level, should they drift
There was a problem hiding this comment.
Gotcha. Maybe add comments on top of the handler functions to explain the difference?
| return "basic" | ||
| } | ||
| return "fancy" | ||
| } |
There was a problem hiding this comment.
Is this preferable to stopping the logger before running the interactive part?
There was a problem hiding this comment.
It felt more robust. Didn't really try the alternative.
There was a problem hiding this comment.
Actually it seems to work just fine.
eysi09
force-pushed
the
artifacts-error
branch
2 times, most recently
from
9119ad4 to
2938099
Compare
eysi09
force-pushed
the
artifacts-error
branch
2 times, most recently
from
548eada to
76563f8
Compare
What this PR does / why we need it:
The first commit fixes an issue with
garden run testreturning exit code 0 if the test fails and artifacts are copied.As I was working on that, I noticed that
garden run taskwould return the wrong exit code for local tasks, and some other inconsistencies in how the outputs were printed. The second commit addresses that.Which issue(s) this PR fixes:
This came up in a conversation on the community Slack.
Special notes for your reviewer:
I turned the
loggerTypestatic property into a function so that commands can dynamically decide what the logger type should be. For example, commands with--interactive=trueshould use the basic logger.Here are screenshots from how the outputs look now: