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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

eskip testable example is incorrectly passing #1860

Closed
ameowlia opened this issue Sep 21, 2021 · 2 comments 路 Fixed by #1861
Closed

eskip testable example is incorrectly passing #1860

ameowlia opened this issue Sep 21, 2021 · 2 comments 路 Fixed by #1861

Comments

@ameowlia
Copy link

馃憢 Hello,

I am investigating the golang issue: golang/go#48362, where testable examples are causing false positives when the output comment block is not the last comment block example.

I ran a parser against the top golang repos, including this repo, and I found this bug with one of your testable examples.

The TODO comment block should be moved before the output comment block like so:

func Example() {
	code := `
		// Skipper - Eskip:
		// a routing table document, containing multiple route definitions
		// route definition to a jsx page renderer
		route0:
			PathRegexp(/\.html$/) && HeaderRegexp("Accept", "text/html") ->
			modPath(/\.html$/, ".jsx") ->
			requestHeader("X-Type", "page") ->
			"https://render.example.org";
		route1: Path("/some/path") -> "https://backend-0.example.org"; // a simple route
		// route definition with a shunt (no backend address)
		route2: Path("/some/other/path") -> static("/", "/var/www") -> <shunt>;
		// route definition directing requests to an api endpoint
		route3:
			Method("POST") && Path("/api") ->
			requestHeader("X-Type", "ajax-post") ->
			"https://api.example.org";
		// route definition with a loopback to route2 (no backend address)
		route4: Path("/some/alternative/path") -> setPath("/some/other/path") -> <loopback>;
        // route definition which forwards rest of requests (no backend address)
		route5: * -> requestHeader("X-Type", "page") -> <forward>;
		`

	routes, err := eskip.Parse(code)
	if err != nil {
		log.Println(err)
		return
	}

	format := "%v: [match] -> [%v filter(s) ->] <%v> \"%v\"\n"
	fmt.Println("Parsed routes:")
	for _, r := range routes {
		fmt.Printf(format, r.Id, len(r.Filters), r.BackendType, r.Backend)
	}
	// TODO: try to represent this compressed output in a nicer way. This is is now somewhat confusing
	// considering that it looks almost like eskip but it isn't.

	// output:
	// Parsed routes:
	// route0: [match] -> [2 filter(s) ->] <network> "https://render.example.org"
	// route1: [match] -> [0 filter(s) ->] <network> "https://backend-0.example.org"
	// route2: [match] -> [1 filter(s) ->] <shunt> ""
	// route3: [match] -> [1 filter(s) ->] <network> "https://api.example.org"
	// route4: [match] -> [1 filter(s) ->] <loopback> ""


}

When I made this change, the example failed:

$ go test .
2021/09/21 19:54:18 parse failed after token >, position 1015: syntax error
--- FAIL: Example (0.00s)
got:

want:
Parsed routes:
route0: [match] -> [2 filter(s) ->] <network> "https://render.example.org"
route1: [match] -> [0 filter(s) ->] <network> "https://backend-0.example.org"
route2: [match] -> [1 filter(s) ->] <shunt> ""
route3: [match] -> [1 filter(s) ->] <network> "https://api.example.org"
route4: [match] -> [1 filter(s) ->] <loopback> ""
FAIL
FAIL    github.com/zalando/skipper/eskip        0.016s
FAIL
AlexanderYastrebov added a commit that referenced this issue Sep 21, 2021
The #1860 pointed out to a bug
golang/go#48362 that leads to false
positive testable example due to trailing comment.

Moreover #918 changed test
input without changing the output and that went unnoticed due to this bug.

This change removes trailing comment to fix the example.
It also removes the test input instead of adding correct output because
the input does not match the final implemenation and thus is misleading.

Fixes #1860

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this issue Sep 21, 2021
The #1860 pointed out a bug
golang/go#48362 that leads to the false
positive testable example caused by a trailing comment.

Moreover #918 changed the test
input without changing the output and that went unnoticed due to this bug.

This change removes trailing comment to fix the example.
It also removes the test input instead of adding a correct output because
the input syntax is incorrect therefore misleading.

Fixes #1860

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

@ameowlia Hello and thank you for detailed report and interesting bug. I have opened #1861 to fix the example.

@szuecs
Copy link
Member

szuecs commented Sep 23, 2021

Thanks @ameowlia !!

AlexanderYastrebov added a commit that referenced this issue Sep 23, 2021
The #1860 pointed out a bug
golang/go#48362 that leads to the false
positive testable example caused by a trailing comment.

Moreover #918 changed the test
input without changing the output and that went unnoticed due to this bug.

This change removes trailing comment to fix the example.
It also removes the test input instead of adding a correct output because
the input syntax is incorrect therefore misleading.

Fixes #1860

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 a pull request may close this issue.

3 participants