diff --git a/checks/cli.go b/checks/cli.go index c16e6db..956d056 100644 --- a/checks/cli.go +++ b/checks/cli.go @@ -85,6 +85,7 @@ func runCLICommandWithLimits( cmd = exec.CommandContext(ctx, "sh", "-c", finalCommand) } + configureCommandCancellation(cmd) cmd.Env = append(os.Environ(), "LANG=en_US.UTF-8") cmd.WaitDelay = commandWaitDelay cancelForOutputLimit := func() { @@ -94,6 +95,8 @@ func runCLICommandWithLimits( stderr := newBoundedBuffer(maxOutputBytesPerStream, cancelForOutputLimit) cmd.Stdout = stdout cmd.Stderr = stderr + stopSignalForwarding := forwardSignalsToCommand(cmd) + defer stopSignalForwarding() err := cmd.Run() if ee, ok := err.(*exec.ExitError); ok { result.ExitCode = ee.ExitCode() diff --git a/checks/command_process_other.go b/checks/command_process_other.go new file mode 100644 index 0000000..9c647f1 --- /dev/null +++ b/checks/command_process_other.go @@ -0,0 +1,11 @@ +//go:build !aix && !darwin && !dragonfly && !freebsd && !linux && !netbsd && !openbsd && !solaris && !windows + +package checks + +import "os/exec" + +func configureCommandCancellation(cmd *exec.Cmd) {} + +func forwardSignalsToCommand(cmd *exec.Cmd) func() { + return func() {} +} diff --git a/checks/command_process_unix.go b/checks/command_process_unix.go new file mode 100644 index 0000000..b286981 --- /dev/null +++ b/checks/command_process_unix.go @@ -0,0 +1,63 @@ +//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris + +package checks + +import ( + "errors" + "os" + "os/exec" + "os/signal" + "sync" + "syscall" +) + +func configureCommandCancellation(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { return killCommandProcessGroup(cmd) } +} + +func forwardSignalsToCommand(cmd *exec.Cmd) func() { + signals := make(chan os.Signal, 1) + done := make(chan struct{}) + signal.Notify(signals, os.Interrupt, syscall.SIGTERM) + var forwardOnce sync.Once + forward := func(received os.Signal) { + forwardOnce.Do(func() { + _ = killCommandProcessGroup(cmd) + signal.Reset(received) + if unixSignal, ok := received.(syscall.Signal); ok { + _ = syscall.Kill(os.Getpid(), unixSignal) + } + }) + } + + go func() { + select { + case received := <-signals: + forward(received) + case <-done: + } + }() + + return func() { + signal.Stop(signals) + select { + case received := <-signals: + forward(received) + default: + } + close(done) + } +} + +func killCommandProcessGroup(cmd *exec.Cmd) error { + if cmd.Process == nil { + return os.ErrProcessDone + } + + err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + if errors.Is(err, syscall.ESRCH) { + return os.ErrProcessDone + } + return err +} diff --git a/checks/command_process_unix_test.go b/checks/command_process_unix_test.go new file mode 100644 index 0000000..51d6579 --- /dev/null +++ b/checks/command_process_unix_test.go @@ -0,0 +1,52 @@ +//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris + +package checks + +import ( + "errors" + "strconv" + "strings" + "syscall" + "testing" + "time" + + api "github.com/bootdotdev/bootdev/client" +) + +func TestRunCLICommandTimeoutKillsDescendants(t *testing.T) { + result := runCLICommandWithLimits( + api.CLIStepCLICommand{Command: "sleep 30 & echo $!; wait"}, + map[string]string{}, + 100*time.Millisecond, + 1024, + ) + if !strings.Contains(result.Err, "command timed out") { + t.Fatalf("command error = %q, want timeout error", result.Err) + } + + pid, err := strconv.Atoi(strings.TrimSpace(result.Stdout)) + if err != nil { + t.Fatalf("child PID output = %q: %v", result.Stdout, err) + } + childAlive := true + t.Cleanup(func() { + if childAlive { + _ = syscall.Kill(pid, syscall.SIGKILL) + } + }) + + deadline := time.Now().Add(time.Second) + for time.Now().Before(deadline) { + err := syscall.Kill(pid, 0) + if errors.Is(err, syscall.ESRCH) { + childAlive = false + return + } + if err != nil { + t.Fatalf("check child process %d: %v", pid, err) + } + time.Sleep(10 * time.Millisecond) + } + + t.Fatalf("child process %d survived command cancellation", pid) +} diff --git a/checks/command_process_windows.go b/checks/command_process_windows.go new file mode 100644 index 0000000..7683909 --- /dev/null +++ b/checks/command_process_windows.go @@ -0,0 +1,38 @@ +//go:build windows + +package checks + +import ( + "errors" + "os" + "os/exec" + "strconv" +) + +func configureCommandCancellation(cmd *exec.Cmd) { + cmd.Cancel = func() error { + if cmd.Process == nil { + return os.ErrProcessDone + } + + treeKill := exec.Command( + "taskkill.exe", + "/PID", strconv.Itoa(cmd.Process.Pid), + "/T", + "/F", + ) + if err := treeKill.Run(); err == nil { + return nil + } + + err := cmd.Process.Kill() + if errors.Is(err, os.ErrProcessDone) { + return os.ErrProcessDone + } + return err + } +} + +func forwardSignalsToCommand(cmd *exec.Cmd) func() { + return func() {} +} diff --git a/cmd/localtest.go b/cmd/localtest.go index 5d64279..554bfbf 100644 --- a/cmd/localtest.go +++ b/cmd/localtest.go @@ -19,7 +19,7 @@ import ( func init() { rootCmd.AddCommand(localTestCmd) - localTestCmd.Flags().BoolVarP(&verboseOutput, "verbose", "v", false, "show detailed output for every step") + localTestCmd.Flags().BoolVarP(&verboseOutput, "verbose", "v", false, "show detailed final output for every step") } var localTestCmd = &cobra.Command{ diff --git a/cmd/run.go b/cmd/run.go index 4032bf0..9e27fb0 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -8,7 +8,7 @@ func init() { rootCmd.AddCommand(runCmd) runCmd.Flags().BoolVarP(&forceSubmit, "submit", "s", false, "shortcut flag to submit after running") runCmd.Flags().BoolVar(&debugSubmission, "debug", false, "log submission request/response debug output") - runCmd.Flags().BoolVarP(&verboseOutput, "verbose", "v", false, "show detailed output for every step") + runCmd.Flags().BoolVarP(&verboseOutput, "verbose", "v", false, "with --submit, show detailed final output for every step") } // runCmd represents the run command diff --git a/cmd/submit.go b/cmd/submit.go index 3e9f9b2..58ad901 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -26,7 +26,7 @@ var ( func init() { rootCmd.AddCommand(submitCmd) submitCmd.Flags().BoolVar(&debugSubmission, "debug", false, "log submission request/response debug output") - submitCmd.Flags().BoolVarP(&verboseOutput, "verbose", "v", false, "show detailed output for every step") + submitCmd.Flags().BoolVarP(&verboseOutput, "verbose", "v", false, "show detailed final output for every step") } // submitCmd represents the submit command @@ -92,14 +92,25 @@ func submissionHandler(cmd *cobra.Command, args []string) error { if err != nil { return err } - checks.ApplySubmissionResults(data, submissionEvent.StructuredErrCLI, ch) + submissionErr := applySubmissionEvent(data, submissionEvent, ch) finalise(submissionEvent) + if submissionErr != nil { + return submissionErr + } } else { finalise(api.LessonSubmissionEvent{}) } return nil } +func applySubmissionEvent(data api.CLIData, event api.LessonSubmissionEvent, ch chan tea.Msg) error { + if event.ResultSlug == api.VerificationResultSlugSystemError { + return errors.New("lesson verification failed due to a system error; please try again") + } + checks.ApplySubmissionResults(data, event.StructuredErrCLI, ch) + return nil +} + func reportDebugFileWrite(path string, err error) { if err != nil { fmt.Fprintf(os.Stderr, "warning: failed to write submission debug output: %v\n", err) diff --git a/cmd/submit_test.go b/cmd/submit_test.go new file mode 100644 index 0000000..0a0909a --- /dev/null +++ b/cmd/submit_test.go @@ -0,0 +1,29 @@ +package cmd + +import ( + "strings" + "testing" + + api "github.com/bootdotdev/bootdev/client" + tea "github.com/charmbracelet/bubbletea" +) + +func TestApplySubmissionEventRejectsSystemErrorWithoutMarkingStepsPassed(t *testing.T) { + data := api.CLIData{Steps: []api.CLIStep{{ + CLICommand: &api.CLIStepCLICommand{Tests: []api.CLICommandTest{{}}}, + }}} + ch := make(chan tea.Msg, 1) + + err := applySubmissionEvent(data, api.LessonSubmissionEvent{ + ResultSlug: api.VerificationResultSlugSystemError, + }, ch) + if err == nil || !strings.Contains(err.Error(), "system error") { + t.Fatalf("applySubmissionEvent() error = %v, want system error", err) + } + + select { + case msg := <-ch: + t.Fatalf("system error unexpectedly emitted result message: %#v", msg) + default: + } +} diff --git a/render/variables.go b/render/variables.go index 21bcf4a..9c2ed29 100644 --- a/render/variables.go +++ b/render/variables.go @@ -13,6 +13,7 @@ import ( type variableEntry struct { name string value string + found bool description string } @@ -27,23 +28,26 @@ func renderVariableSection(title string, entries []variableEntry) string { var str strings.Builder fmt.Fprintf(&str, " %s: \n", title) for _, entry := range entries { - fmt.Fprintf(&str, " - %s: %s (%s)\n", entry.name, formatVariableValue(entry.value), entry.description) + fmt.Fprintf(&str, " - %s: %s (%s)\n", entry.name, formatVariableValue(entry.value, entry.found), entry.description) } return str.String() } -func formatVariableValue(value string) string { - if value == "" { +func formatVariableValue(value string, found bool) string { + if !found { return "[not found]" } + if value == "" { + return "[empty]" + } return value } func savedVariablesForHTTPResult(result api.HTTPRequestResult) []variableEntry { var entries []variableEntry for _, responseVariable := range result.Request.ResponseVariables { - value := result.Variables[responseVariable.Name] - if value == "" { + value, found := result.Variables[responseVariable.Name] + if !found { continue } @@ -51,17 +55,19 @@ func savedVariablesForHTTPResult(result api.HTTPRequestResult) []variableEntry { entries = append(entries, variableEntry{ name: responseVariable.Name, value: value, + found: true, description: description, }) } for _, responseHeaderVariable := range result.Request.ResponseHeaderVariables { - value := result.Variables[responseHeaderVariable.Name] - if value == "" { + value, found := result.Variables[responseHeaderVariable.Name] + if !found { continue } entries = append(entries, variableEntry{ name: responseHeaderVariable.Name, value: value, + found: true, description: responseHeaderVariableDescription(responseHeaderVariable), }) } @@ -71,7 +77,7 @@ func savedVariablesForHTTPResult(result api.HTTPRequestResult) []variableEntry { func missingSaveVariablesForHTTPResult(result api.HTTPRequestResult) []variableEntry { var entries []variableEntry for _, responseVariable := range result.Request.ResponseVariables { - if result.Variables[responseVariable.Name] != "" { + if _, found := result.Variables[responseVariable.Name]; found { continue } @@ -82,7 +88,7 @@ func missingSaveVariablesForHTTPResult(result api.HTTPRequestResult) []variableE }) } for _, responseHeaderVariable := range result.Request.ResponseHeaderVariables { - if result.Variables[responseHeaderVariable.Name] != "" { + if _, found := result.Variables[responseHeaderVariable.Name]; found { continue } entries = append(entries, variableEntry{ @@ -108,7 +114,7 @@ func availableVariablesForHTTPResult(result api.HTTPRequestResult) (entries []va return } expectsVariables = true - value := result.Variables[name] + value, found := result.Variables[name] key := name + "\x00" + description if seen[key] { return @@ -117,6 +123,7 @@ func availableVariablesForHTTPResult(result api.HTTPRequestResult) (entries []va entries = append(entries, variableEntry{ name: name, value: value, + found: found, description: description, }) } @@ -167,7 +174,7 @@ func availableVariablesForCLIResult(result api.CLICommandResult) (entries []vari add := func(name, description string) { expectsVariables = true - value := result.Variables[name] + value, found := result.Variables[name] key := name + "\x00" + description if seen[key] { return @@ -176,6 +183,7 @@ func availableVariablesForCLIResult(result api.CLICommandResult) (entries []vari entries = append(entries, variableEntry{ name: name, value: value, + found: found, description: description, }) } diff --git a/render/variables_test.go b/render/variables_test.go index 55abbf5..b7ed73e 100644 --- a/render/variables_test.go +++ b/render/variables_test.go @@ -83,13 +83,48 @@ func TestAvailableVariablesPrintsNotFoundWhenExpectedButUnavailable(t *testing.T } } +func TestHTTPVariableSectionsDistinguishEmptyFromMissing(t *testing.T) { + result := api.HTTPRequestResult{ + Variables: map[string]string{"emptyCode": ""}, + Request: api.CLIStepHTTPRequest{ + ResponseVariables: []api.HTTPRequestResponseVariable{ + {Name: "emptyCode", Path: ".empty_code"}, + {Name: "missingCode", Path: ".missing_code"}, + }, + Request: api.HTTPRequest{ + Headers: map[string]string{"X-Code": "${emptyCode}"}, + }, + }, + } + + got := printHTTPRequestResult(result) + for _, expected := range []string{ + "emptyCode: [empty] (JSON Body .empty_code)", + "missingCode: [not found] (JSON Body .missing_code)", + "emptyCode: [empty] (Request Header \"X-Code\")", + } { + if !strings.Contains(got, expected) { + t.Errorf("output missing %q\n%s", expected, got) + } + } + for _, unexpected := range []string{ + "emptyCode: [not found]", + "missingCode: [empty]", + } { + if strings.Contains(got, unexpected) { + t.Errorf("output unexpectedly contains %q\n%s", unexpected, got) + } + } +} + func TestCLIAvailableVariables(t *testing.T) { result := api.CLICommandResult{ Variables: map[string]string{ - "url": "http://localhost:42069", + "empty": "", + "url": "http://localhost:42069", }, Command: api.CLIStepCLICommand{ - Command: "curl ${url}", + Command: "curl ${url} ${empty}", Tests: []api.CLICommandTest{ {StdoutContainsAll: []string{"${expected}"}}, }, @@ -105,6 +140,9 @@ func TestCLIAvailableVariables(t *testing.T) { if !strings.Contains(got, "url: http://localhost:42069 (Command)") { t.Fatalf("expected url entry in:\n%s", got) } + if !strings.Contains(got, "empty: [empty] (Command)") { + t.Fatalf("expected empty entry in:\n%s", got) + } if !strings.Contains(got, "expected: [not found] (Stdout Contains Test)") { t.Fatalf("expected missing expected entry in:\n%s", got) } diff --git a/render/view.go b/render/view.go index b6fa82b..85ebb3e 100644 --- a/render/view.go +++ b/render/view.go @@ -110,7 +110,7 @@ func (m rootModel) View() string { break } - showAllDetails := m.verbose || (!m.isSubmit && m.finalized) + showAllDetails := m.finalized && (m.verbose || !m.isSubmit) failed := step.passed != nil && !*step.passed if showAllDetails { str.WriteString(renderTestHeader(step.description, m.spinner, step.finished, m.isSubmit, step.passed, step.noPenaltyOnFail)) @@ -168,6 +168,17 @@ func (m rootModel) View() string { str.WriteString(green.Render("Return to your browser to continue with the next lesson.")) str.WriteByte('\n') str.WriteByte('\n') + } else if m.result == api.VerificationResultSlugSystemError { + str.WriteByte('\n') + str.WriteByte('\n') + str.WriteString(red.Render("Unable to verify this lesson due to a system error.")) + if m.failure != nil && m.failure.ErrorMessage != "" { + str.WriteString(red.Render(fmt.Sprintf("\nError: %s", m.failure.ErrorMessage))) + } + str.WriteByte('\n') + str.WriteString(red.Render("Please try again.")) + str.WriteByte('\n') + str.WriteByte('\n') } else if m.result == api.VerificationResultSlugNoop { str.WriteString("\n\nTests failed! ❌") fmt.Fprintf(&str, "\n\nFailed Step: %v", m.failure.FailedStepIndex+1) diff --git a/render/view_test.go b/render/view_test.go index 9f8551c..6703506 100644 --- a/render/view_test.go +++ b/render/view_test.go @@ -125,6 +125,52 @@ func TestVerboseViewShowsSuccessfulDetails(t *testing.T) { } } +func TestVerboseViewStaysCompactUntilFinalized(t *testing.T) { + m := initModel(true, true) + m.steps = []stepModel{{ + description: "The command prints a greeting", + detail: "Command: echo hello", + finished: true, + tests: []testModel{{text: "Expect stdout to contain all of: hello", finished: true}}, + }} + + view := m.View() + if !strings.Contains(view, "The command prints a greeting") { + t.Fatalf("view missing compact step description\n%s", view) + } + for _, unexpected := range []string{"Command: echo hello", "Expect stdout to contain all of: hello"} { + if strings.Contains(view, unexpected) { + t.Errorf("view unexpectedly contains %q before finalization\n%s", unexpected, view) + } + } +} + +func TestSystemErrorViewDoesNotShowStepsAsPassed(t *testing.T) { + m := initModel(true, false) + m.finalized = true + m.result = api.VerificationResultSlugSystemError + m.steps = []stepModel{{ + description: "The command prints a greeting", + finished: true, + }} + + view := m.View() + for _, expected := range []string{ + "? The command prints a greeting", + "Unable to verify this lesson due to a system error.", + "Please try again.", + } { + if !strings.Contains(view, expected) { + t.Errorf("view missing %q\n%s", expected, view) + } + } + for _, unexpected := range []string{"✓ The command prints a greeting", "All tests passed!"} { + if strings.Contains(view, unexpected) { + t.Errorf("view unexpectedly contains %q\n%s", unexpected, view) + } + } +} + func TestStartStepFallsBackToTechnicalDescription(t *testing.T) { m := initModel(true, false) updated, _ := m.Update(messages.StartStepMsg{CMD: "go test ./..."})