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.

Sign up for GitHub

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

Jump to bottom

cmd/go: improve pattern matching in script_test stdout, stderr and grep commands #46157

Open
perillo opened this issue May 13, 2021 · 12 comments
Open

cmd/go: improve pattern matching in script_test stdout, stderr and grep commands #46157

perillo opened this issue May 13, 2021 · 12 comments
Labels
GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@perillo
Copy link
Contributor

perillo commented May 13, 2021

What version of Go are you using (go version)?

$ go version

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

Proposal

I noted that the script tests in cmd/go/testdata/script often are not very readable.

These are the changes I want to propose and implement:

  1. It is not simple to specify the os.PathSeparator in a pattern for the stdout, stderr and grep command, and [/\\] should be used. For multiple or long paths this reduce readability.
    As an example from mod_replace.txt:

    stdout '^rsc.io/quote/v3 v3.0.0 '$GOPATH'[/\\]pkg[/\\]mod[/\\]not-rsc.io[/\\]quote@v0.1.0-nomod '$GOPATH'[/\\]pkg[/\\]mod[/\\]cache[/\\]download[/\\]not-rsc.io[/\\]quote[/\\]@v[/\\]v0.1.0-nomod.mod => not-rsc.io/quote v0.1.0-nomod '$GOPATH'[/\\]pkg[/\\]mod[/\\]not-rsc.io[/\\]quote@v0.1.0-nomod '$GOPATH'[/\\]pkg[/\\]mod[/\\]cache[/\\]download[/\\]not-rsc.io[/\\]quote[/\\]@v[/\\]v0.1.0-nomod.mod$'

    In https://go-review.googlesource.com/c/go/+/319409 I proposed to use the · (middle dot) character, that is replaced to os.PathSeparator when the new -path flag is specified for the stdout, stderr or grep commands.

    A better solution is to use the ${/} environment variable expansion, to parallel ${:} for os.PathListSeparator.
    No one will probably use the middle dot character.

  2. In a script test, only single quoted string are supported, and environment variables expansion is not allowed.

    I propose to add support for double quoted string, so that, as an example:

    '^no Go files in '${WORK}${/}'gopath'$/'src'$/'dir'$/'subdir$'

    can be replaced with the more readable:

    "^no Go files in ${WORK}${/}gopath${/}src${/}dir${/}subdir$"

  3. Many tests don't escape the . character when used in semver and file path.

    I propose to add support for a raw string, where the string content is quoted using regexp.QuoteMeta.
    Note that raw newline and tab characters are not allowed.

    This change has low priority, since it is probably ok to not escape the . character.
@heschi heschi added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go labels May 13, 2021
@heschi heschi added this to the Backlog milestone May 13, 2021
@heschi
Copy link
Contributor

heschi commented May 13, 2021

cc @bcmills @jayconrod @matloob

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/319312 mentions this issue: cmd/go: add double-quoted strings to script_test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/320309 mentions this issue: cmd/go: add special parameter support in script_test

@jayconrod
Copy link
Contributor

These proposed changes seem like overkill to me. While they allow more precision, they make tests more difficult to read and write. For tests, I think it's more important to prioritize readability and writability. I don't expect the added precision will help us catch many regressions.

Something simpler would be better, based on one of your earlier changes: perhaps adding a -path parameter to the grep, stdout, and stderr commands. If -path is set, script_test would replace os.PathSeparator with / and os.PathListSeparator with :. Then checks could be written like 'foo/bar:baz/quux' instead of "foo$/bar$:baz$:quux".

@perillo
Copy link
Contributor Author

perillo commented May 17, 2021 •
edited
Loading

The two CL I proposed are not for allowing more precision, but to improve readability for tests like

stdout '^rsc.io/quote/v3 v3.0.0 '$GOPATH'[/\\]pkg[/\\]mod[/\\]not-rsc.io[/\\]quote@v0.1.0-nomod '$GOPATH'[/\\]pkg[/\\]mod[/\\]cache[/\\]download[/\\]not-rsc.io[/\\]quote[/\\]@v[/\\]v0.1.0-nomod.mod => not-rsc.io/quote v0.1.0-nomod '$GOPATH'[/\\]pkg[/\\]mod[/\\]not-rsc.io[/\\]quote@v0.1.0-nomod '$GOPATH'[/\\]pkg[/\\]mod[/\\]cache[/\\]download[/\\]not-rsc.io[/\\]quote[/\\]@v[/\\]v0.1.0-nomod.mod$'

With the the 2 proposed CL:

stdout "^rsc.io/quote/v3 v3.0.0 $GOPATH$/pkg$/mod$/not-rsc.io$/quote@v0.1.0-nomod $GOPATH$/pkg$/mod$/cache$/download$/not-rsc.io$/quote$/@v$/v0.1.0-nomod.mod => not-rsc.io/quote v0.1.0-nomod $GOPATH$/pkg$/mod$/not-rsc.io$/quote@v0.1.0-nomod $GOPATH$/pkg$/mod$/cache$/download$/not-rsc.io$/quote$/@v$/v0.1.0-nomod.mod$"

Note that:

  1. The slashes in the URL are not expanded
  2. There is no need to leave the quoted string when expanding an environment variable
  3. It is possible to test if the pattern is correct both on UNIX and Windows systems.
    This is very important, IMHO. This (with 1.) was the reason why I used the
    middle dot character instead of the slash in the original proposal.

I think this is much more readable compared to the current tests.

@perillo
Copy link
Contributor Author

perillo commented May 17, 2021

Also, the : character is often used in error messages, so it has the same problems as the / character.

@jayconrod
Copy link
Contributor

This still doesn't seem readable to me. I'd much rather write:

stdout -path '^rsc.io/quote/v3 v3.0.0 '$GOPATH'/pkg/mod/not-rsc.io/quote@v0.1.0-nomod '$GOPATH'/pkg/mod/cache/download/not-rsc.io/quote/@v/v0.1.0-nomod.mod => not-rsc.io/quote v0.1.0-nomod '$GOPATH'/pkg/mod/not-rsc.io/quote@v0.1.0-nomod '$GOPATH'/pkg/mod/cache/download/not-rsc.io/quote/@v/v0.1.0-nomod.mod$'

@perillo
Copy link
Contributor Author

perillo commented May 17, 2021

The problem is that, on Windows, rsc.io/quote/v3 will be replaced with rsc.io\quote\v3.
It is possible to solve the problem by disallowing the replacement of / in single quoted strings and allowing it in double quoted strings, but I suspect that this will confuse the programmers.

The current and proposed syntax follow the UNIX shell syntax, so many programmers should be comfortable with it.

@jayconrod
Copy link
Contributor

The problem is that, on Windows, rsc.io/quote/v3 will be replaced with rsc.io\quote\v3.

I'm suggesting the opposite actually: replace backslashes with slashes in the output text before matching. So $WORK/foo would match against something like C:/work/foo. Expressions like $WORK/foo would work on any platform with -path being the only needed decoration.

The current and proposed syntax follow the UNIX shell syntax, so many programmers should be comfortable with it.

How would this handle $ as the end-of-line regular expression operator? I'm a bit uncomfortable mixing these syntaxes, more than they are already mixed.

@perillo
Copy link
Contributor Author

perillo commented May 17, 2021

The problem is that, on Windows, rsc.io/quote/v3 will be replaced with rsc.io\quote\v3.

I'm suggesting the opposite actually: replace backslashes with slashes in the output text before matching. So $WORK/foo would match against something like C:/work/foo. Expressions like $WORK/foo would work on any platform with -path being the only needed decoration.

I'm a bit confused. On Windows, shouldn't the path be C:\work\foo?

The current and proposed syntax follow the UNIX shell syntax, so many programmers should be comfortable with it.

How would this handle $ as the end-of-line regular expression operator? I'm a bit uncomfortable mixing these syntaxes, more than they are already mixed.

The variables are expanded before the matching, so there are no problems. The single $ is never expanded; the same for $c, where c is a single character that is not not a letter or digit, with the special case of / and :.

@perillo
Copy link
Contributor Author

perillo commented May 17, 2021 •
edited
Loading

Here is an example:
https://play.golang.org/p/voG_Tz4cDm7

@perillo
Copy link
Contributor Author

perillo commented May 17, 2021

Well, a single digit is expanded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@jayconrod @perillo @gopherbot @heschi