Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/run-tests.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: run-tests

on: [push]
on: [push, pull_request]

jobs:
test:
Expand Down
95 changes: 55 additions & 40 deletions cmd/svcinit/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ func main() {
must(err)

var testCmd *exec.Cmd
testCtx, testCancel := context.WithCancel(ctx)
testErrCh := make(chan error, 1)
if testLabel != "" {
testArgs := os.Args[1:]
Expand All @@ -205,7 +206,7 @@ func main() {
}
testStartTime := time.Now()

testCmd = exec.CommandContext(ctx, testPath, testArgs...)
testCmd = exec.CommandContext(testCtx, testPath, testArgs...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex will this testCmd also get stopped/cancelled when the parent ctx is cancelled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When one context is derived from an outer context, as queryCtx is derived from ctx in this example, if the outer context is canceled, then the derived context is automatically canceled as well.

https://go.dev/doc/database/cancel-operations

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah perfect, I suspected as much but was curious if I could get Codex to explain it. Guess not :)

testCmd.Env = testEnv

// Adjust remaining timeout to account for service startup.
Expand All @@ -216,7 +217,7 @@ func main() {
fmt.Println(err)
} else {
timeoutVal -= int(math.Ceil(testStartTime.Sub(start).Seconds()))
testCmd.Env = append(testCmd.Env, "TEST_TIMEOUT=" + strconv.Itoa(timeoutVal))
testCmd.Env = append(testCmd.Env, "TEST_TIMEOUT="+strconv.Itoa(timeoutVal))
}
}

Expand All @@ -237,7 +238,59 @@ func main() {

fmt.Println()

if shouldHotReload && !enablePerServiceReload {
fmt.Println()
fmt.Println("###########################################################################################")
fmt.Println(" Detected that you are running under ibazel, but do not have per-service-reload enabled.")
fmt.Println(" In this configuration, services will not be restarted when their code changes.")
fmt.Println(" If this was unintentional, you can retry with per-service-reload enabled:")
fmt.Println("")
fmt.Printf(" `bazel run --@rules_itest//:enable_per_service_reload %s`\n", testLabel)
fmt.Println("###########################################################################################")
fmt.Println()
fmt.Println()
}

select {
case <-ctx.Done():
log.Println("Shutting down services.")
_, err := r.StopAll()
must(err)
log.Println("Cleaning up.")
return
case ibazelCmd := <-interactiveCh:
log.Println(ibazelCmd)

// Restart any services as needed.
unversionedSpecs, err := readServiceSpecs(serviceSpecsPath)
must(err)

serviceSpecs, err := augmentServiceSpecs(unversionedSpecs, ports, svcctlPortStr)
must(err)

testCancel()

// TODO(zbarsky): what is the right behavior here when services are crashing in ibazel mode?

// This is a brittle way of draining a channel in a nonblocking way,
// consider instead signalling cancellation of the services with a
// context, letting them close the channel, and using a waitgroup to
// wait for them to exit.
Drain:
for {
select {
case <-servicesErrCh:
// nothing
default:
break Drain
}
}

criticalPath, err = r.UpdateSpecsAndRestart(serviceSpecs, servicesErrCh, []byte(ibazelCmd))
must(err)

continue

case testErr := <-testErrCh:
if testErr != nil {
log.Printf("Encountered error during test run: %s\n", testErr)
Expand Down Expand Up @@ -282,44 +335,6 @@ func main() {
if isOneShot {
break
}

if shouldHotReload && !enablePerServiceReload {
fmt.Println()
fmt.Println()
fmt.Println("###########################################################################################")
fmt.Println(" Detected that you are running under ibazel, but do not have per-service-reload enabled.")
fmt.Println(" In this configuration, services will not be restarted when their code changes.")
fmt.Println(" If this was unintentional, you can retry with per-service-reload enabled:")
fmt.Println("")
fmt.Printf(" `bazel run --@rules_itest//:enable_per_service_reload %s`\n", testLabel)
fmt.Println("###########################################################################################")
fmt.Println()
fmt.Println()
}

select {
case <-ctx.Done():
log.Println("Shutting down services.")
_, err := r.StopAll()
must(err)
log.Println("Cleaning up.")
return
case ibazelCmd := <-interactiveCh:
log.Println(ibazelCmd)

// Restart any services as needed.
unversionedSpecs, err := readServiceSpecs(serviceSpecsPath)
must(err)

serviceSpecs, err := augmentServiceSpecs(unversionedSpecs, ports, svcctlPortStr)
must(err)

// TODO(zbarsky): what is the right behavior here when services are crashing in ibazel mode?
for range servicesErrCh {
} // Drain the channel
criticalPath, err = r.UpdateSpecsAndRestart(serviceSpecs, servicesErrCh, []byte(ibazelCmd))
must(err)
}
}
}

Expand Down