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

eskip: re-generate parser #2450

Merged
merged 1 commit into from Jul 12, 2023
Merged

Conversation

AlexanderYastrebov
Copy link
Member

go tool cover fails for eskip package:

$ go test ./eskip/ -coverprofile=.coverprofile && go tool cover -func .coverprofile
ok      github.com/zalando/skipper/eskip        0.026s  coverage: 88.2% of statements
cover: inconsistent NumStmt: changed from 1 to 2

this apparently happens due to malformed .coverprofile containing non-go files

$ cut -d: -f1 .coverprofile | grep -v -F '.go' | sort -u
github.com/zalando/skipper/eskip/parser.y
github.com/zalando/skipper/eskip/yaccpar
mode

added by line directives in generated parser.go.

The problem looks similar to the issue golang/go#45361.

To fix the problem updated go:generate directive to omit line directives (-l) and parsing tables (-v "") and re-generated the parser:

$ rm $(which goyacc)
$ go install golang.org/x/tools/cmd/goyacc
$ go generate ./eskip

Benchmark results did not change after re-generate:

name               old time/op    new time/op    delta
ParsePredicates-8    4.58µs ± 7%    4.42µs ± 0%   ~     (p=0.064 n=10+8)
LBBackendString-8    2.40µs ±12%    2.53µs ±11%   ~     (p=0.060 n=10+10)

name               old alloc/op   new alloc/op   delta
ParsePredicates-8      744B ± 0%      744B ± 0%   ~     (all equal)
LBBackendString-8    4.65kB ± 0%    4.65kB ± 0%   ~     (all equal)

name               old allocs/op  new allocs/op  delta
ParsePredicates-8      35.0 ± 0%      35.0 ± 0%   ~     (all equal)
LBBackendString-8      12.0 ± 0%      12.0 ± 0%   ~     (all equal)

go tool cover fails for eskip package:
```
$ go test ./eskip/ -coverprofile=.coverprofile && go tool cover -func .coverprofile
ok      github.com/zalando/skipper/eskip        0.026s  coverage: 88.2% of statements
cover: inconsistent NumStmt: changed from 1 to 2
```

this apparently happens due to malformed .coverprofile containing non-go files
```sh
$ cut -d: -f1 .coverprofile | grep -v -F '.go' | sort -u
github.com/zalando/skipper/eskip/parser.y
github.com/zalando/skipper/eskip/yaccpar
mode
```
added by line directives in generated parser.go.

The problem looks similar to the issue golang/go#45361.

To fix the problem updated go:generate directive to omit line directives (-l) and
parsing tables (-v "") and re-generated the parser:
```sh
$ rm $(which goyacc)
$ go install golang.org/x/tools/cmd/goyacc
$ go generate ./eskip
```

Benchmark results did not change after re-generate:
```
name               old time/op    new time/op    delta
ParsePredicates-8    4.58µs ± 7%    4.42µs ± 0%   ~     (p=0.064 n=10+8)
LBBackendString-8    2.40µs ±12%    2.53µs ±11%   ~     (p=0.060 n=10+10)

name               old alloc/op   new alloc/op   delta
ParsePredicates-8      744B ± 0%      744B ± 0%   ~     (all equal)
LBBackendString-8    4.65kB ± 0%    4.65kB ± 0%   ~     (all equal)

name               old allocs/op  new allocs/op  delta
ParsePredicates-8      35.0 ± 0%      35.0 ± 0%   ~     (all equal)
LBBackendString-8      12.0 ± 0%      12.0 ± 0%   ~     (all equal)
```

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@RomanZavodskikh
Copy link
Member

👍

1 similar comment
@MustafaSaber
Copy link
Member

👍

@AlexanderYastrebov AlexanderYastrebov merged commit 2a38f0e into master Jul 12, 2023
13 checks passed
@AlexanderYastrebov AlexanderYastrebov deleted the eskip/regenerate-parser branch July 12, 2023 10:17
AlexanderYastrebov added a commit that referenced this pull request Jul 12, 2023
Use ./... instead of loop over packages.

Followup on #2449 and #2450

Also:
* increases routestring Example timeout after observing sporadic failures

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Jul 12, 2023
Use ./... instead of loop over packages.

Followup on #2449 and #2450

Also:
* increases routestring Example timeout after observing sporadic failures

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
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.

None yet

3 participants