From 79a64efbfdcc7db1207d79154f3b985d9260fc8b Mon Sep 17 00:00:00 2001 From: Boming Zhang Date: Sun, 2 Aug 2026 01:51:04 -0700 Subject: [PATCH] test: cover stage and executor reliability regressions --- cmd/joj3/conf/conf_test.go | 36 +++++-- cmd/joj3/stage_test.go | 44 +++++++++ internal/executor/local/executor_test.go | 52 ++++++++++ internal/executor/sandbox/convert_test.go | 50 ++++++++++ internal/executor/sandbox/executor_test.go | 87 ++++++++++++++++ internal/stage/run_test.go | 109 +++++++++++++++++++++ 6 files changed, 369 insertions(+), 9 deletions(-) create mode 100644 internal/executor/local/executor_test.go diff --git a/cmd/joj3/conf/conf_test.go b/cmd/joj3/conf/conf_test.go index 712c7e9..58de74d 100644 --- a/cmd/joj3/conf/conf_test.go +++ b/cmd/joj3/conf/conf_test.go @@ -132,14 +132,32 @@ func TestParseConventionalCommit(t *testing.T) { } func TestMatchGroupsUsesExactTokens(t *testing.T) { - conf := &Conf{Stages: []ConfStage{ - {Name: "short", Groups: []string{"c"}}, - {Name: "cpp", Groups: []string{"cpp"}}, - {Name: "lint", Groups: []string{"lint"}}, - }} - got := MatchGroups(conf, &ConventionalCommit{Group: "cpp, lint"}) - want := []string{"cpp", "lint"} - if !reflect.DeepEqual(got, want) { - t.Fatalf("MatchGroups() = %v, want %v", got, want) + tests := []struct { + name string + group string + want []string + }{ + {name: "exact not substring", group: "cpp", want: []string{"cpp"}}, + {name: "case insensitive", group: "CPP", want: []string{"cpp"}}, + {name: "comma and space", group: "cpp, lint", want: []string{"cpp", "lint"}}, + {name: "semicolon", group: "cpp;lint", want: []string{"cpp", "lint"}}, + {name: "pipe", group: "cpp|lint", want: []string{"cpp", "lint"}}, + {name: "tab", group: "cpp\tlint", want: []string{"cpp", "lint"}}, + {name: "duplicate token", group: "cpp,cpp", want: []string{"cpp"}}, + {name: "all", group: "ALL", want: []string{"c", "cpp", "lint"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + conf := &Conf{Stages: []ConfStage{ + {Name: "short", Groups: []string{"c"}}, + {Name: "cpp", Groups: []string{"cpp"}}, + {Name: "lint", Groups: []string{"lint"}}, + }} + got := MatchGroups(conf, &ConventionalCommit{Group: tt.group}) + if !reflect.DeepEqual(got, tt.want) { + t.Fatalf("MatchGroups() = %v, want %v", got, tt.want) + } + }) } } diff --git a/cmd/joj3/stage_test.go b/cmd/joj3/stage_test.go index e83cd4b..9bd4a8f 100644 --- a/cmd/joj3/stage_test.go +++ b/cmd/joj3/stage_test.go @@ -1,6 +1,8 @@ package main import ( + "os" + "path/filepath" "strings" "testing" @@ -25,3 +27,45 @@ func TestRunStagesPreservesMainAndOutputErrors(t *testing.T) { t.Fatalf("runStages() error = %v, want main and output errors", err) } } + +func TestRunStagesPreservesPhaseErrorsAndWritesMainFailure(t *testing.T) { + broken := func(name, executor string) conf.ConfStage { + s := conf.ConfStage{Name: name} + s.Executor.Name = executor + return s + } + outputPath := filepath.Join(t.TempDir(), "result.json") + c := &conf.Conf{ + SandboxExecServer: "localhost:5051", + OutputPath: outputPath, + PreStages: []conf.ConfStage{broken("pre", "missing-pre")}, + Stages: []conf.ConfStage{broken("main", "missing-main")}, + PostStages: []conf.ConfStage{broken("post", "missing-post")}, + } + callbackCalled := false + _, forceQuit, err := runStages(c, nil, func(results []stage.StageResult, forceQuit string) { + callbackCalled = true + if len(results) != 1 || results[0].Name != "Internal Error" || forceQuit != "Internal Error" { + t.Fatalf("callback results = %v, forceQuit = %q", results, forceQuit) + } + }) + if err == nil { + t.Fatal("runStages() unexpectedly succeeded") + } + for _, want := range []string{"missing-pre", "missing-main", "missing-post"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("runStages() error %q does not contain %q", err, want) + } + } + if forceQuit != "Internal Error" || !callbackCalled { + t.Fatalf("forceQuit = %q, callbackCalled = %v", forceQuit, callbackCalled) + } + content, readErr := os.ReadFile(outputPath) + if readErr != nil { + t.Fatal(readErr) + } + if !strings.Contains(string(content), "Internal Error") || + !strings.Contains(string(content), "missing-main") { + t.Fatalf("output = %s", content) + } +} diff --git a/internal/executor/local/executor_test.go b/internal/executor/local/executor_test.go new file mode 100644 index 0000000..136d318 --- /dev/null +++ b/internal/executor/local/executor_test.go @@ -0,0 +1,52 @@ +package local + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/joint-online-judge/JOJ3/internal/stage" +) + +func TestRunRejectsEmptyArgs(t *testing.T) { + _, err := (&Local{}).Run(context.Background(), []stage.Cmd{{}}) + if err == nil || err.Error() != "command args must not be empty" { + t.Fatalf("Run() error = %v", err) + } +} + +func TestRunCancellationKillsProcessGroup(t *testing.T) { + dir := t.TempDir() + ready := filepath.Join(dir, "ready") + marker := filepath.Join(dir, "marker") + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { + _, err := (&Local{}).Run(ctx, []stage.Cmd{{ + Args: []string{"/bin/sh", "-c", "touch \"$1\"; (sleep 0.3; touch \"$2\") & wait", "sh", ready, marker}, + }}) + done <- err + }() + + deadline := time.Now().Add(2 * time.Second) + for { + if _, err := os.Stat(ready); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("command did not start") + } + time.Sleep(10 * time.Millisecond) + } + cancel() + if err := <-done; !errors.Is(err, context.Canceled) { + t.Fatalf("Run() error = %v, want context canceled", err) + } + time.Sleep(500 * time.Millisecond) + if _, err := os.Stat(marker); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("child process survived cancellation: %v", err) + } +} diff --git a/internal/executor/sandbox/convert_test.go b/internal/executor/sandbox/convert_test.go index b5181ad..f3b56f6 100644 --- a/internal/executor/sandbox/convert_test.go +++ b/internal/executor/sandbox/convert_test.go @@ -1,6 +1,9 @@ package sandbox import ( + "fmt" + "os" + "path/filepath" "strings" "testing" @@ -19,3 +22,50 @@ func TestConvertPBCmdReturnsSourceReadError(t *testing.T) { t.Fatalf("convertPBCmd() error = %v, want source read error", err) } } + +func TestConvertPBCmdReturnsStandardFileReadError(t *testing.T) { + missing := t.TempDir() + "/missing" + _, err := convertPBCmd([]stage.Cmd{{ + Args: []string{"true"}, + Stdin: &stage.CmdFile{Src: &missing}, + }}) + if err == nil || !strings.Contains(err.Error(), "standard file") || + !strings.Contains(err.Error(), "read source file") { + t.Fatalf("convertPBCmd() error = %v, want standard source read error", err) + } +} + +func TestConvertPBCmdReturnsCopyInDirectoryWalkError(t *testing.T) { + missing := t.TempDir() + "/missing" + _, err := convertPBCmd([]stage.Cmd{{ + Args: []string{"true"}, + CopyInDir: missing, + }}) + if err == nil || !strings.Contains(err.Error(), "walk") { + t.Fatalf("convertPBCmd() error = %v, want directory walk error", err) + } +} + +func TestConvertPBCmdPreservesMoreThanSCMMaxFDFileCount(t *testing.T) { + const fileCount = 301 + dir := t.TempDir() + for i := range fileCount { + path := filepath.Join(dir, fmt.Sprintf("file-%03d", i)) + if err := os.WriteFile(path, []byte("content"), 0o600); err != nil { + t.Fatal(err) + } + } + + cmds, err := convertPBCmd([]stage.Cmd{{ + Args: []string{"true"}, + CopyIn: make(map[string]stage.CmdFile), + CopyInDir: dir, + }}) + if err != nil { + t.Fatal(err) + } + if len(cmds) != 1 || len(cmds[0].GetCopyIn()) != fileCount { + t.Fatalf("converted %d commands with %d files, want 1 command with %d files", + len(cmds), len(cmds[0].GetCopyIn()), fileCount) + } +} diff --git a/internal/executor/sandbox/executor_test.go b/internal/executor/sandbox/executor_test.go index 0a164aa..eab6285 100644 --- a/internal/executor/sandbox/executor_test.go +++ b/internal/executor/sandbox/executor_test.go @@ -1,13 +1,36 @@ package sandbox import ( + "context" + "errors" "math" "testing" "time" + "github.com/criyle/go-judge/pb" "github.com/joint-online-judge/JOJ3/internal/stage" + "google.golang.org/grpc" + "google.golang.org/protobuf/types/known/emptypb" ) +type fakeExecutorClient struct { + pb.ExecutorClient + exec func(context.Context, *pb.Request) (*pb.Response, error) + fileDelete func(context.Context, *pb.FileID) (*emptypb.Empty, error) +} + +func (f fakeExecutorClient) Exec( + ctx context.Context, req *pb.Request, _ ...grpc.CallOption, +) (*pb.Response, error) { + return f.exec(ctx, req) +} + +func (f fakeExecutorClient) FileDelete( + ctx context.Context, id *pb.FileID, _ ...grpc.CallOption, +) (*emptypb.Empty, error) { + return f.fileDelete(ctx, id) +} + func TestExecRPCTimeout(t *testing.T) { tests := []struct { name string @@ -25,6 +48,16 @@ func TestExecRPCTimeout(t *testing.T) { cmds: []stage.Cmd{{CPULimit: uint64(time.Minute)}}, want: 2*time.Minute + rpcTimeoutMargin, }, + { + name: "clock limit takes precedence over cpu limit", + cmds: []stage.Cmd{{ClockLimit: uint64(time.Minute), CPULimit: uint64(10 * time.Minute)}}, + want: time.Minute + rpcTimeoutMargin, + }, + { + name: "cpu multiplication overflow", + cmds: []stage.Cmd{{CPULimit: math.MaxUint64}}, + want: time.Duration(math.MaxInt64), + }, { name: "duration overflow", cmds: []stage.Cmd{{ClockLimit: math.MaxUint64}}, @@ -40,3 +73,57 @@ func TestExecRPCTimeout(t *testing.T) { }) } } + +func TestRunAppliesComputedRPCDeadline(t *testing.T) { + var remaining time.Duration + client := fakeExecutorClient{ + exec: func(ctx context.Context, _ *pb.Request) (*pb.Response, error) { + deadline, ok := ctx.Deadline() + if !ok { + t.Fatal("Exec context has no deadline") + } + remaining = time.Until(deadline) + return &pb.Response{}, nil + }, + } + executor := &Sandbox{ + execClient: client, + cachedMap: make(map[string]string), + } + want := 2*time.Minute + rpcTimeoutMargin + _, err := executor.Run(context.Background(), []stage.Cmd{{ + Args: []string{"true"}, + CPULimit: uint64(time.Minute), + }}) + if err != nil { + t.Fatal(err) + } + if remaining > want || remaining < want-time.Second { + t.Fatalf("RPC deadline remaining = %v, want approximately %v", remaining, want) + } +} + +func TestCleanupJoinsDeleteErrorsAndClearsCache(t *testing.T) { + deleteErr := errors.New("delete failed") + deleted := 0 + client := fakeExecutorClient{ + fileDelete: func(context.Context, *pb.FileID) (*emptypb.Empty, error) { + deleted++ + return nil, deleteErr + }, + } + executor := &Sandbox{ + execClient: client, + cachedMap: map[string]string{"one": "1", "two": "2"}, + } + err := executor.Cleanup(context.Background()) + if !errors.Is(err, deleteErr) { + t.Fatalf("Cleanup() error = %v, want delete error", err) + } + if deleted != 2 { + t.Fatalf("FileDelete called %d times, want 2", deleted) + } + if len(executor.cachedMap) != 0 { + t.Fatalf("cached files not cleared: %v", executor.cachedMap) + } +} diff --git a/internal/stage/run_test.go b/internal/stage/run_test.go index 3d9103c..cfed0f7 100644 --- a/internal/stage/run_test.go +++ b/internal/stage/run_test.go @@ -2,6 +2,7 @@ package stage import ( "context" + "errors" "strings" "testing" ) @@ -21,6 +22,10 @@ func (contractTestParser) Run([]ExecutorResult, any) ([]ParserResult, bool, erro } func TestRunRejectsParserResultCountMismatch(t *testing.T) { + originalExecutors, originalParsers := executorMap, parserMap + executorMap, parserMap = map[string]Executor{}, map[string]Parser{} + t.Cleanup(func() { executorMap, parserMap = originalExecutors, originalParsers }) + const executorName = "contract-test-executor" const parserName = "contract-test-parser" RegisterExecutor(executorName, contractTestExecutor{}) @@ -38,3 +43,107 @@ func TestRunRejectsParserResultCountMismatch(t *testing.T) { t.Fatalf("Run() force quit = %q, want test", forceQuit) } } + +type executorFunc struct { + run func(context.Context, []Cmd) ([]ExecutorResult, error) + cleanup func(context.Context) error +} + +func (e executorFunc) Run(ctx context.Context, cmds []Cmd) ([]ExecutorResult, error) { + return e.run(ctx, cmds) +} + +func (e executorFunc) Cleanup(ctx context.Context) error { + if e.cleanup == nil { + return nil + } + return e.cleanup(ctx) +} + +type parserFunc func([]ExecutorResult, any) ([]ParserResult, bool, error) + +func (p parserFunc) Run(results []ExecutorResult, conf any) ([]ParserResult, bool, error) { + return p(results, conf) +} + +func isolateRegistries(t *testing.T) { + t.Helper() + originalExecutors, originalParsers := executorMap, parserMap + executorMap, parserMap = map[string]Executor{}, map[string]Parser{} + t.Cleanup(func() { executorMap, parserMap = originalExecutors, originalParsers }) +} + +func TestCleanupJoinsExecutorErrors(t *testing.T) { + isolateRegistries(t) + errOne := errors.New("cleanup one") + errTwo := errors.New("cleanup two") + RegisterExecutor("one", executorFunc{ + run: func(context.Context, []Cmd) ([]ExecutorResult, error) { return nil, nil }, + cleanup: func(context.Context) error { return errOne }, + }) + RegisterExecutor("two", executorFunc{ + run: func(context.Context, []Cmd) ([]ExecutorResult, error) { return nil, nil }, + cleanup: func(context.Context) error { return errTwo }, + }) + + err := Cleanup(context.Background()) + if !errors.Is(err, errOne) || !errors.Is(err, errTwo) { + t.Fatalf("Cleanup() error = %v, want both cleanup errors", err) + } +} + +func TestRunStopsAfterParserContractViolation(t *testing.T) { + isolateRegistries(t) + RegisterExecutor("executor", executorFunc{ + run: func(context.Context, []Cmd) ([]ExecutorResult, error) { + return []ExecutorResult{{Status: StatusAccepted}}, nil + }, + }) + RegisterParser("bad", parserFunc(func([]ExecutorResult, any) ([]ParserResult, bool, error) { + return nil, false, nil + })) + secondCalled := false + RegisterParser("second", parserFunc(func([]ExecutorResult, any) ([]ParserResult, bool, error) { + secondCalled = true + return []ParserResult{{}}, false, nil + })) + + _, forceQuit, err := Run(context.Background(), []Stage{{ + Name: "contract", + Executor: StageExecutor{Name: "executor", Cmds: []Cmd{{}}}, + Parsers: []StageParser{{Name: "bad"}, {Name: "second"}}, + }}) + if err == nil || forceQuit != "contract" { + t.Fatalf("Run() = forceQuit %q, error %v", forceQuit, err) + } + if secondCalled { + t.Fatal("parser after contract violation was called") + } +} + +func TestRunSetsForceQuitForMissingComponents(t *testing.T) { + tests := []struct { + name string + stage Stage + }{ + {name: "executor", stage: Stage{Name: "missing-executor", Executor: StageExecutor{Name: "unknown"}}}, + { + name: "parser", + stage: Stage{ + Name: "missing-parser", + Executor: StageExecutor{Name: "executor", Cmds: []Cmd{{}}}, + Parsers: []StageParser{{Name: "unknown"}}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isolateRegistries(t) + RegisterExecutor("executor", contractTestExecutor{}) + _, forceQuit, err := Run(context.Background(), []Stage{tt.stage}) + if err == nil || forceQuit != tt.stage.Name { + t.Fatalf("Run() = forceQuit %q, error %v", forceQuit, err) + } + }) + } +}