Skip to content
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

x/perf/benchfmt: output an error in case the input file is not utf8 encoded #58579

Open
phenpessoa opened this issue Feb 17, 2023 · 4 comments · May be fixed by golang/perf#9
Open

x/perf/benchfmt: output an error in case the input file is not utf8 encoded #58579

phenpessoa opened this issue Feb 17, 2023 · 4 comments · May be fixed by golang/perf#9
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@phenpessoa
Copy link

phenpessoa commented Feb 17, 2023

Related to: #58576

Currently, if a benchmark file is encoded in utf16 (which is the default in windows), the benchstat tool will fail silently. This might be hard to debug to the end user.

I propose that instead of failing silently, the tool should return an error letting the user know their file is not properly encoded.

On windows, running this command: go test -bench . -count 10 > old.txt will return a utf16 encoded file by default.

It is a simple change, after this line, we just need to add something like:

if !utf8.Valid(line) {
	r.err = fmt.Errorf("%s: invalid encode, only utf8 encoded files are supported", r.result.fileName)
	return false
}

And then if the user were to run the benchstat tool, they would get that error instead of it failing silently.

@gopherbot gopherbot added this to the Proposal milestone Feb 17, 2023
@prattmic
Copy link
Member

cc @aclements

@phenpessoa phenpessoa changed the title proposal: x/perf/cmd/benchfmt: output an error in case the input file is not utf8 encoded proposal: x/perf/benchfmt: output an error in case the input file is not utf8 encoded Feb 17, 2023
@ianlancetaylor
Copy link
Contributor

I think this is just a bug fix, taking it out of the proposal process.

@ianlancetaylor ianlancetaylor changed the title proposal: x/perf/benchfmt: output an error in case the input file is not utf8 encoded x/perf/benchfmt: output an error in case the input file is not utf8 encoded Feb 17, 2023
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Feb 17, 2023
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unreleased Feb 17, 2023
@phenpessoa
Copy link
Author

Can I send my patch for code review or does this issue need to be in the NeedsFix state?

@seankhliao
Copy link
Member

just send it in

phenpessoa added a commit to phenpessoa/perf that referenced this issue Mar 13, 2023
Currently, when using the benchstat tool to read files that are not UTF-8 encoded, it will fail silently.
With this patch, when parsing non UTF-8 encoded files, benchfmt will now return an error instead letting the user know their input file is not properly formatted.

Fixes golang/go#58579
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants