Skip to content

Conversation

@jelias2
Copy link
Contributor

@jelias2 jelias2 commented Oct 29, 2025

Description

  • More metrics and strip escape characters, and whitespaces, and optionally line-numbers from source test from go test output

Tests

  • Unit tests have been added + running locally with output below

Additional context

Logs Before

Screenshot 2025-10-29 at 5 02 09 PM

Logs After

Screenshot 2025-10-29 at 6 11 34 PM

Log File output

Screenshot 2025-10-29 at 11 58 44 PM

Metadata

  • Fixes #[Link to Issue]

@jelias2 jelias2 requested a review from a team as a code owner October 29, 2025 20:49
@jelias2 jelias2 requested a review from bitwiseguy October 29, 2025 20:49
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 41.36126% with 112 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.49%. Comparing base (65745cb) to head (dba41c2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
op-acceptor/nat.go 19.51% 66 Missing ⚠️
op-acceptor/config.go 0.00% 24 Missing ⚠️
op-acceptor/metrics/metrics.go 0.00% 13 Missing ⚠️
op-acceptor/runner/runner.go 76.31% 8 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #499      +/-   ##
==========================================
- Coverage   58.58%   58.49%   -0.10%     
==========================================
  Files          91       91              
  Lines       13020    13129     +109     
==========================================
+ Hits         7628     7680      +52     
- Misses       4942     4997      +55     
- Partials      450      452       +2     
Flag Coverage Δ
op-acceptor 60.80% <41.36%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
op-acceptor/flags/flags.go 81.81% <ø> (ø)
op-acceptor/logging/filelogger.go 79.62% <100.00%> (+0.52%) ⬆️
op-acceptor/runner/runner.go 73.60% <76.31%> (+1.70%) ⬆️
op-acceptor/metrics/metrics.go 72.94% <0.00%> (-13.17%) ⬇️
op-acceptor/config.go 0.00% <0.00%> (ø)
op-acceptor/nat.go 38.85% <19.51%> (-3.52%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 1414 to 1415
// Trim leading and trailing whitespace (including newlines)
cleaned = strings.TrimSpace(cleaned)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to remove newlines? I'm just wondering if this result in hard to read output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've posted some before and after logs in the PR description, feel free to take a look and provide feedback

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the images - really helpful!

I find this to be an improvement for console logs but perhaps a step back for file logs. The file:num can be useful information and we've also lost the "usual" go test formatting/indentation, which makes it seem odd. What do you think?
Should we treat them differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that file:num is useful, in the PR there is a flag for toggling this StripCodeLinePrefixes.

I personally found the go test formatting in the file logs not the most readable. See some logs from running prior.

I'm in favor of the change still if you want can put it behind a flag for like --intelligent-formatting or something.

Screenshot 2025-10-30 at 1 39 47 AM Screenshot 2025-10-30 at 1 40 21 AM

Copy link
Contributor Author

@jelias2 jelias2 Oct 30, 2025

Choose a reason for hiding this comment

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

Screenshot 2025-10-30 at 1 44 03 AM I see what you mean about the error output, will take a look

}

// cleanTestOutput cleans up test output by removing ANSI codes, excessive whitespace, and optionally file:line prefixes
func cleanTestOutput(output string, stripFileLinePrefixes bool) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some unit tests to ensure this function behaves as expected?

GoBinary string // path to the Go binary
AllowSkips bool // Whether to allow skipping tests when preconditions are not met
OutputRealtimeLogs bool // Whether to output test logs to the console
StripCodeLinePrefixes bool // Whether to strip file:line prefixes from test logs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only additional config, large diff is indentation

goBinary string // Path to the Go binary
allowSkips bool // Whether to allow skipping tests when preconditions are not met
outputRealtimeLogs bool // If enabled, test logs will be outputted in realtime
stripCodeLinePrefixes bool // Whether to strip file:line prefixes from test logs
Copy link
Contributor Author

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

goBinary: cfg.GoBinary,
allowSkips: cfg.AllowSkips,
outputRealtimeLogs: cfg.OutputRealtimeLogs,
stripCodeLinePrefixes: cfg.StripCodeLinePrefixes,
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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

DefaultTimeout: ctx.Duration(flags.DefaultTimeout.Name),
Timeout: ctx.Duration(flags.Timeout.Name),
OutputRealtimeLogs: ctx.Bool(flags.OutputRealtimeLogs.Name),
StripCodeLinePrefixes: ctx.Bool(flags.StripCodeLinePrefixes.Name),
Copy link
Contributor Author

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

GoBinary: config.GoBinary,
AllowSkips: config.AllowSkips,
OutputRealtimeLogs: config.OutputRealtimeLogs,
StripCodeLinePrefixes: config.StripCodeLinePrefixes,
Copy link
Contributor Author

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

@jelias2 jelias2 force-pushed the jelias2/op-acceptor-metrics-improvements branch from 5da44ac to f1f867e Compare October 30, 2025 04:10
@jelias2 jelias2 force-pushed the jelias2/op-acceptor-metrics-improvements branch from f1f867e to eb71708 Compare October 30, 2025 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants