Skip to content

fix(op-acceptor): add build dependencies and increase logs #352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
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
10 changes: 9 additions & 1 deletion op-acceptor/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ COPY op-acceptor/ .

RUN just build

FROM alpine:3.21
FROM golang:1.23.5-alpine3.21

RUN apk add --no-cache ca-certificates build-base git

# Install Go binary
# (we copy Go directly from the builder stage for simplicity and consistency)
Expand All @@ -19,6 +21,7 @@ ENV GOPATH="/go"
ENV PATH="${GOPATH}/bin:${PATH}"

WORKDIR /app

RUN addgroup -S app && adduser -S app -G app && \
mkdir -p /devnets && \
chown -R app:app /devnets
Expand All @@ -30,7 +33,12 @@ chown -R app:app /go

COPY --from=builder /app/bin/op-acceptor /app/
COPY op-acceptor/example-validators.yaml /app/

# Set ownership of the /app directory to allow the application to write logs and other files at runtime
RUN chown -R app:app /app/
Comment on lines +37 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? The logs and files are working without this, I believe?

Copy link
Author

Choose a reason for hiding this comment

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

This is needed if running the docker image locally unless we change the directory we run it on (like in CI).


USER app

VOLUME /devnets

ENTRYPOINT ["/app/op-acceptor"]
7 changes: 5 additions & 2 deletions op-acceptor/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ type Config struct {
AllowSkips bool // Allow tests to be skipped instead of failing when preconditions are not met
DefaultTimeout time.Duration // Default timeout for individual tests, can be overridden by test config
LogDir string // Directory to store test logs

Log log.Logger
OutputTestLogs bool // Whether to output test logs to the console
TestLogLevel string // Log level to be used for the tests
Log log.Logger
}

// NewConfig creates a new Config instance
Expand Down Expand Up @@ -72,6 +73,8 @@ func NewConfig(ctx *cli.Context, log log.Logger, testDir string, validatorConfig
RunOnce: runOnce,
AllowSkips: ctx.Bool(flags.AllowSkips.Name),
DefaultTimeout: ctx.Duration(flags.DefaultTimeout.Name),
OutputTestLogs: ctx.Bool(flags.OutputTestLogs.Name),
TestLogLevel: ctx.String(flags.TestLogLevel.Name),
LogDir: logDir,
Log: log,
}, nil
Expand Down
14 changes: 14 additions & 0 deletions op-acceptor/flags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,18 @@ var (
EnvVars: opservice.PrefixEnvVar(EnvVarPrefix, "LOGDIR"),
Usage: "Directory to store test logs. Defaults to 'logs' if not specified.",
}
TestLogLevel = &cli.StringFlag{
Name: "test-log-level",
Value: "info",
EnvVars: opservice.PrefixEnvVar(EnvVarPrefix, "TEST_LOG_LEVEL"),
Usage: "Log level to be used for the tests. Defaults to 'info'.",
}
OutputTestLogs = &cli.BoolFlag{
Name: "output-test-logs",
Value: false,
EnvVars: opservice.PrefixEnvVar(EnvVarPrefix, "OUTPUT_TEST_LOGS"),
Usage: "Output test logs to the console. Defaults to false.",
}
)

var requiredFlags = []cli.Flag{
Expand All @@ -80,6 +92,8 @@ var optionalFlags = []cli.Flag{
AllowSkips,
DefaultTimeout,
LogDir,
TestLogLevel,
OutputTestLogs,
}
var Flags []cli.Flag

Expand Down
18 changes: 10 additions & 8 deletions op-acceptor/nat.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,16 @@ func New(ctx context.Context, config *Config, version string, shutdownCallback f

// Create runner with registry
testRunner, err := runner.NewTestRunner(runner.Config{
Registry: reg,
WorkDir: config.TestDir,
Log: config.Log,
TargetGate: config.TargetGate,
GoBinary: config.GoBinary,
AllowSkips: config.AllowSkips,
NetworkName: networkName,
DevnetEnv: env,
Registry: reg,
WorkDir: config.TestDir,
Log: config.Log,
TargetGate: config.TargetGate,
GoBinary: config.GoBinary,
AllowSkips: config.AllowSkips,
OutputTestLogs: config.OutputTestLogs,
TestLogLevel: config.TestLogLevel,
NetworkName: networkName,
DevnetEnv: env,
})
if err != nil {
return nil, fmt.Errorf("failed to create test runner: %w", err)
Expand Down
132 changes: 132 additions & 0 deletions op-acceptor/runner/logging_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package runner

import (
"context"
"os"
"path/filepath"
"testing"

"github.com/ethereum-optimism/infra/op-acceptor/types"
Expand Down Expand Up @@ -68,3 +70,133 @@ gates:
assert.Contains(t, failingTest.Stdout, "This is some stdout output that should be captured")
assert.Contains(t, failingTest.Stdout, "This is a second line of output")
}

// TestLogLevelEnvironment verifies that the TEST_LOG_LEVEL environment variable is correctly set and used
func TestLogLevelEnvironment(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also test the different levels behave differently?

ctx := context.Background()

// Create a simple test file in the work directory
testContent := []byte(`
package main

import (
"os"
"testing"
)

func TestLogLevelEnvironment(t *testing.T) {
// Get log level from environment
logLevel := os.Getenv("TEST_LOG_LEVEL")
if logLevel == "" {
t.Log("TEST_LOG_LEVEL not set")
} else {
t.Log("TEST_LOG_LEVEL set to", logLevel)
}
}
`)
configContent := []byte(`
gates:
- id: logging-gate
description: "Gate with a test that outputs logs"
suites:
logging-suite:
description: "Suite with a test that outputs logs"
tests:
- name: TestLogLevelEnvironment
package: "./main"
`)

r := setupTestRunner(t, testContent, configContent)
r.testLogLevel = "debug"
err := os.WriteFile(filepath.Join(r.workDir, "main_test.go"), testContent, 0644)
require.NoError(t, err)

result, err := r.RunTest(ctx, types.ValidatorMetadata{
ID: "test1",
Gate: "logging-gate",
FuncName: "TestLogLevelEnvironment",
Package: ".",
})

require.NoError(t, err)
assert.Equal(t, types.TestStatusPass, result.Status)
assert.Equal(t, "test1", result.Metadata.ID)
assert.Equal(t, "logging-gate", result.Metadata.Gate)
assert.Equal(t, ".", result.Metadata.Package)
assert.False(t, result.Metadata.RunAll)
assert.Contains(t, result.Stdout, "TEST_LOG_LEVEL set to debug")
}

// TestOutputTestLogs verifies that test logs are properly captured and output when enabled
func TestOutputTestLogs(t *testing.T) {
// Create a test file that outputs logs at different levels
testContent := []byte(`
package feature_test

import (
"testing"
)

func TestWithLogs(t *testing.T) {
t.Log("This is a test output")
}
`)

configContent := []byte(`
gates:
- id: logging-gate
description: "Gate with a test that outputs logs"
suites:
logging-suite:
description: "Suite with a test that outputs logs"
tests:
- name: TestWithLogs
package: "./feature"
`)

// Setup the test runner with OutputTestLogs enabled
r := setupTestRunner(t, testContent, configContent)
r.outputTestLogs = true

// Run the test
result, err := r.RunAllTests(context.Background())
require.NoError(t, err)
assert.Equal(t, types.TestStatusPass, result.Status)

// Verify the structure
require.Contains(t, result.Gates, "logging-gate")
gate := result.Gates["logging-gate"]
require.Contains(t, gate.Suites, "logging-suite")
suite := gate.Suites["logging-suite"]

// Get the test result
var testResult *types.TestResult
for _, test := range suite.Tests {
testResult = test
break
}
require.NotNil(t, testResult)

// Verify the test passed and has logs captured
assert.Equal(t, types.TestStatusPass, testResult.Status)
assert.NotEmpty(t, testResult.Stdout)
assert.Contains(t, testResult.Stdout, "This is a test output")

// Now run with OutputTestLogs disabled
r.outputTestLogs = false
result, err = r.RunAllTests(context.Background())
require.NoError(t, err)
assert.Equal(t, types.TestStatusPass, result.Status)

// Get the test result again
var testResult2 *types.TestResult
for _, test := range result.Gates["logging-gate"].Suites["logging-suite"].Tests {
testResult2 = test
break
}
require.NotNil(t, testResult)

// Verify that logs are not captured when OutputTestLogs is disabled
assert.Equal(t, types.TestStatusPass, testResult2.Status)
assert.Contains(t, testResult2.Stdout, "This is a test output")
Comment on lines +199 to +201
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be asserting that it does NOT contain this output?

Suggested change
// Verify that logs are not captured when OutputTestLogs is disabled
assert.Equal(t, types.TestStatusPass, testResult2.Status)
assert.Contains(t, testResult2.Stdout, "This is a test output")
// Verify that logs are not captured when OutputTestLogs is disabled
assert.Equal(t, types.TestStatusPass, testResult2.Status)
assert.NotContains(t, testResult2.Stdout, "This is a test output")

}
Loading