generated from ethereum-optimism/.github
-
Notifications
You must be signed in to change notification settings - Fork 104
Jelias2/op acceptor metrics improvements #499
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
jelias2
wants to merge
5
commits into
main
Choose a base branch
from
jelias2/op-acceptor-metrics-improvements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+664
−167
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8b51b21
feat: test log string colors and escape chars, add more metrics
jelias2 32630dc
feat: linting
jelias2 eb71708
feat: add tests and stip-line-log-prefixes flag
jelias2 de56e31
feat: enable whitespace trimming
jelias2 dba41c2
feat: tests updated for whitespace trimming
jelias2 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,29 +15,30 @@ import ( | |
|
|
||
| // Config holds the application configuration | ||
| type Config struct { | ||
| TestDir string | ||
| ValidatorConfig string | ||
| TargetGate string | ||
| GatelessMode bool | ||
| GoBinary string | ||
| RunInterval time.Duration // Interval between test runs | ||
| RunOnce bool // Indicates if the service should exit after one test run | ||
| 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 | ||
| Timeout time.Duration // Timeout for gateless mode tests (if specified) | ||
| LogDir string // Directory to store test logs | ||
| OutputRealtimeLogs bool // If enabled, test logs will be outputted in realtime | ||
| TestLogLevel string // Log level to be used for the tests | ||
| Orchestrator flags.OrchestratorType // Devstack orchestrator type | ||
| DevnetEnvURL string // URL or path to the devnet environment file | ||
| Serial bool // Whether to run tests serially instead of in parallel | ||
| Concurrency int // Number of concurrent test workers (0 = auto-determine) | ||
| ShowProgress bool // Whether to show periodic progress updates during test execution | ||
| ProgressInterval time.Duration // Interval between progress updates when ShowProgress is 'true' | ||
| FlakeShake bool // Enable flake-shake mode for test stability validation | ||
| FlakeShakeIterations int // Number of times to run each test in flake-shake mode | ||
| Log log.Logger | ||
| ExcludeGates []string // List of gate IDs whose tests should be excluded | ||
| TestDir string | ||
| ValidatorConfig string | ||
| TargetGate string | ||
| GatelessMode bool | ||
| GoBinary string | ||
| RunInterval time.Duration // Interval between test runs | ||
| RunOnce bool // Indicates if the service should exit after one test run | ||
| 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 | ||
| Timeout time.Duration // Timeout for gateless mode tests (if specified) | ||
| LogDir string // Directory to store test logs | ||
| OutputRealtimeLogs bool // If enabled, test logs will be outputted in realtime | ||
| StripCodeLinePrefixes bool // If enabled, file:line prefixes will be stripped from test logs | ||
| TestLogLevel string // Log level to be used for the tests | ||
| Orchestrator flags.OrchestratorType // Devstack orchestrator type | ||
| DevnetEnvURL string // URL or path to the devnet environment file | ||
| Serial bool // Whether to run tests serially instead of in parallel | ||
| Concurrency int // Number of concurrent test workers (0 = auto-determine) | ||
| ShowProgress bool // Whether to show periodic progress updates during test execution | ||
| ProgressInterval time.Duration // Interval between progress updates when ShowProgress is 'true' | ||
| FlakeShake bool // Enable flake-shake mode for test stability validation | ||
| FlakeShakeIterations int // Number of times to run each test in flake-shake mode | ||
| Log log.Logger | ||
| ExcludeGates []string // List of gate IDs whose tests should be excluded | ||
| } | ||
|
|
||
| // NewConfig creates a new Config from cli context | ||
|
|
@@ -114,29 +115,30 @@ func NewConfig(ctx *cli.Context, log log.Logger, testDir string, validatorConfig | |
| } | ||
|
|
||
| return &Config{ | ||
| TestDir: absTestDir, | ||
| ValidatorConfig: absValidatorConfig, | ||
| TargetGate: gate, | ||
| GatelessMode: gatelessMode, | ||
| GoBinary: ctx.String(flags.GoBinary.Name), | ||
| RunInterval: runInterval, | ||
| RunOnce: runOnce, | ||
| AllowSkips: ctx.Bool(flags.AllowSkips.Name), | ||
| DefaultTimeout: ctx.Duration(flags.DefaultTimeout.Name), | ||
| Timeout: ctx.Duration(flags.Timeout.Name), | ||
| OutputRealtimeLogs: ctx.Bool(flags.OutputRealtimeLogs.Name), | ||
| TestLogLevel: ctx.String(flags.TestLogLevel.Name), | ||
| Orchestrator: orchestrator, | ||
| DevnetEnvURL: devnetEnvURL, | ||
| Serial: ctx.Bool(flags.Serial.Name), | ||
| Concurrency: ctx.Int(flags.Concurrency.Name), | ||
| ShowProgress: ctx.Bool(flags.ShowProgress.Name), | ||
| ProgressInterval: ctx.Duration(flags.ProgressInterval.Name), | ||
| FlakeShake: ctx.Bool(flags.FlakeShake.Name), | ||
| FlakeShakeIterations: ctx.Int(flags.FlakeShakeIterations.Name), | ||
| LogDir: logDir, | ||
| Log: log, | ||
| ExcludeGates: excludeGates, | ||
| TestDir: absTestDir, | ||
| ValidatorConfig: absValidatorConfig, | ||
| TargetGate: gate, | ||
| GatelessMode: gatelessMode, | ||
| GoBinary: ctx.String(flags.GoBinary.Name), | ||
| RunInterval: runInterval, | ||
| RunOnce: runOnce, | ||
| AllowSkips: ctx.Bool(flags.AllowSkips.Name), | ||
| DefaultTimeout: ctx.Duration(flags.DefaultTimeout.Name), | ||
| Timeout: ctx.Duration(flags.Timeout.Name), | ||
| OutputRealtimeLogs: ctx.Bool(flags.OutputRealtimeLogs.Name), | ||
| StripCodeLinePrefixes: ctx.Bool(flags.StripCodeLinePrefixes.Name), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only additional config StripCodeLinePrefixes, large diff is indentation |
||
| TestLogLevel: ctx.String(flags.TestLogLevel.Name), | ||
| Orchestrator: orchestrator, | ||
| DevnetEnvURL: devnetEnvURL, | ||
| Serial: ctx.Bool(flags.Serial.Name), | ||
| Concurrency: ctx.Int(flags.Concurrency.Name), | ||
| ShowProgress: ctx.Bool(flags.ShowProgress.Name), | ||
| ProgressInterval: ctx.Duration(flags.ProgressInterval.Name), | ||
| FlakeShake: ctx.Bool(flags.FlakeShake.Name), | ||
| FlakeShakeIterations: ctx.Int(flags.FlakeShakeIterations.Name), | ||
| LogDir: logDir, | ||
| Log: log, | ||
| ExcludeGates: excludeGates, | ||
| }, nil | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,227 @@ | ||
| package logging | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestCleanLogOutput(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| expected string | ||
| }{ | ||
| { | ||
| name: "empty string", | ||
| input: "", | ||
| expected: "", | ||
| }, | ||
| { | ||
| name: "simple text without special characters", | ||
| input: "hello world", | ||
| expected: "hello world", | ||
| }, | ||
| { | ||
| name: "text with leading and trailing whitespace", | ||
| input: " hello world ", | ||
| expected: "hello world", | ||
| }, | ||
| { | ||
| name: "text with multiple spaces", | ||
| input: "hello world", | ||
| expected: "hello world", | ||
| }, | ||
| { | ||
| name: "text with tabs", | ||
| input: "hello\t\tworld", | ||
| expected: "hello world", | ||
| }, | ||
| { | ||
| name: "text with newlines", | ||
| input: "hello\nworld", | ||
| expected: "hello world", | ||
| }, | ||
| { | ||
| name: "text with mixed whitespace", | ||
| input: "hello \t\n world", | ||
| expected: "hello world", | ||
| }, | ||
| { | ||
| name: "file:line prefix simple", | ||
| input: "test.go:123: error message", | ||
| expected: "error message", | ||
| }, | ||
| { | ||
| name: "file:line prefix with underscore", | ||
| input: "da_footprint_test.go:188: some log output", | ||
| expected: "some log output", | ||
| }, | ||
| { | ||
| name: "file:line prefix with leading whitespace", | ||
| input: " test_file.go:456: message here", | ||
| expected: "message here", | ||
| }, | ||
| { | ||
| name: "file:line prefix with hyphen", | ||
| input: "my-file.go:99: content", | ||
| expected: "content", | ||
| }, | ||
| { | ||
| name: "ANSI color codes", | ||
| input: "\x1b[31mred text\x1b[0m", | ||
| expected: "red text", | ||
| }, | ||
| { | ||
| name: "ANSI codes with other formatting", | ||
| input: "\x1b[1;32mbold green\x1b[0m normal", | ||
| expected: "bold green normal", | ||
| }, | ||
| { | ||
| name: "combination: file prefix, ANSI codes, and whitespace", | ||
| input: " test.go:42: \x1b[31m error: failed\x1b[0m ", | ||
| expected: "error: failed", | ||
| }, | ||
| { | ||
| name: "combination: multiple whitespace types", | ||
| input: "test.go:1: hello\t\t world \n foo", | ||
| expected: "hello world foo", | ||
| }, | ||
| { | ||
| name: "text that looks like but isn't a file:line prefix", | ||
| input: "this is not file.go:123: a prefix because text comes before", | ||
| expected: "this is not file.go:123: a prefix because text comes before", | ||
| }, | ||
| { | ||
| name: "multiple lines with different prefixes", | ||
| input: "file1.go:10: first line\nfile2.go:20: second line", | ||
| expected: "first line file2.go:20: second line", | ||
| }, | ||
| { | ||
| name: "no file extension in prefix", | ||
| input: "file:123: should not match", | ||
| expected: "file:123: should not match", | ||
| }, | ||
| { | ||
| name: "wrong extension in prefix", | ||
| input: "file.txt:123: should not match", | ||
| expected: "file.txt:123: should not match", | ||
| }, | ||
| { | ||
| name: "real-world example with geth logger output", | ||
| input: " da_test.go:188: \x1b[36mINFO\x1b[0m [01-01|00:00:00.000] Transaction submitted hash=0x123", | ||
| expected: "INFO [01-01|00:00:00.000] Transaction submitted hash=0x123", | ||
| }, | ||
| { | ||
| name: "multiple consecutive newlines", | ||
| input: "line1\n\n\nline2", | ||
| expected: "line1 line2", | ||
| }, | ||
| { | ||
| name: "only whitespace", | ||
| input: " \t\n ", | ||
| expected: "", | ||
| }, | ||
| { | ||
| name: "unicode characters preserved", | ||
| input: "hello 世界 🌍", | ||
| expected: "hello 世界 🌍", | ||
| }, | ||
| { | ||
| name: "file prefix at start preserves rest of content", | ||
| input: "test.go:1: key=value another=thing", | ||
| expected: "key=value another=thing", | ||
| }, | ||
| { | ||
| name: "real log from da_footprint_test", | ||
| input: " da_footprint_test.go:188: INFO [10-29|23:42:54.881] \"Block 0xbf9f2a47f13a639f439c3bb04070d3186019972b78d694e56e4e61647d4433a0:228357 has DA footprint (0) <= gasUsed (46218), trying next...\"", | ||
| expected: "INFO [10-29|23:42:54.881] \"Block 0xbf9f2a47f13a639f439c3bb04070d3186019972b78d694e56e4e61647d4433a0:228357 has DA footprint (0) <= gasUsed (46218), trying next...\"", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := CleanLogOutput(tt.input, true, true) | ||
| assert.Equal(t, tt.expected, result, "CleanLogOutput(%q, true, true) = %q, want %q", tt.input, result, tt.expected) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestCleanLogOutputWithoutStrippingPrefixes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| expected string | ||
| }{ | ||
| { | ||
| name: "file:line prefix preserved", | ||
| input: "da_footprint_test.go:188: some log output", | ||
| expected: "da_footprint_test.go:188: some log output", | ||
| }, | ||
| { | ||
| name: "file:line prefix preserved with whitespace collapsed", | ||
| input: "test.go:1: hello\t\t world \n foo", | ||
| expected: "test.go:1: hello world foo", | ||
| }, | ||
| { | ||
| name: "ANSI codes still stripped", | ||
| input: "test.go:42: \x1b[31mred text\x1b[0m", | ||
| expected: "test.go:42: red text", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := CleanLogOutput(tt.input, false, true) | ||
| assert.Equal(t, tt.expected, result, "CleanLogOutput(%q, false, true) = %q, want %q", tt.input, result, tt.expected) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestCleanLogOutputRegexPatterns(t *testing.T) { | ||
| t.Run("fileLinePrefixRegex matches valid patterns", func(t *testing.T) { | ||
| validPatterns := []string{ | ||
| "test.go:123: ", | ||
| " test.go:1: ", | ||
| "my_file.go:999: ", | ||
| "My-File.go:1: ", | ||
| "FILE123.go:456: ", | ||
| } | ||
| for _, pattern := range validPatterns { | ||
| assert.True(t, fileLinePrefixRegex.MatchString(pattern), | ||
| "fileLinePrefixRegex should match %q", pattern) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("fileLinePrefixRegex does not match invalid patterns", func(t *testing.T) { | ||
| invalidPatterns := []string{ | ||
| "file.txt:123: ", // not .go file | ||
| "prefix file.go:123: ", // not at start | ||
| "file.go:abc: ", // not a number | ||
| "file:123: ", // missing .go | ||
| ".go:123: ", // no filename | ||
| "file.go:", // missing line number | ||
| } | ||
| for _, pattern := range invalidPatterns { | ||
| assert.False(t, fileLinePrefixRegex.MatchString(pattern), | ||
| "fileLinePrefixRegex should not match %q", pattern) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("multipleWhitespaceRegex collapses various whitespace", func(t *testing.T) { | ||
| testCases := []struct { | ||
| input string | ||
| expected string | ||
| }{ | ||
| {"a b", "a b"}, | ||
| {"a\tb", "a b"}, | ||
| {"a\nb", "a b"}, | ||
| {"a\r\nb", "a b"}, | ||
| {"a \t\n b", "a b"}, | ||
| } | ||
| for _, tc := range testCases { | ||
| result := multipleWhitespaceRegex.ReplaceAllString(tc.input, " ") | ||
| assert.Equal(t, tc.expected, result) | ||
| } | ||
| }) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only additional config StripCodeLinePrefixes, large diff is indentation