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

test: add end2end testing for checkers #8

Closed
quasilyte opened this issue May 6, 2018 · 7 comments
Closed

test: add end2end testing for checkers #8

quasilyte opened this issue May 6, 2018 · 7 comments
Assignees
Milestone

Comments

@quasilyte
Copy link
Member

We need to test that checkers output does match expectations.

I like how end2end testing is done in Go, see go vet tests for example.

It's possible to grab "errcheck" functionality used there for our purposes,
or we can adapt something else (or roll our own).

@quasilyte quasilyte added this to the v0.1 milestone May 7, 2018
@quasilyte quasilyte added the help wanted Extra attention is needed label May 7, 2018
@quasilyte quasilyte self-assigned this May 8, 2018
@quasilyte quasilyte removed the help wanted Extra attention is needed label May 9, 2018
@quasilyte
Copy link
Member Author

quasilyte commented May 9, 2018

The proposed test format is this (tests for param-name checker):

//>Loud:0 consider `in' name instead of `IN'
//>Loud:1 consider `out' name instead of `OUT'
func f1(IN int) (OUT int) { //<Loud:0,Loud:1
	return 0
}

//>Loud:2 consider `in' name instead of `IN'
//>Capitalized:0 `X' should not be capitalized
//>Capitalized:1 `Y' should not be capitalized
//>Capitalized:2 `Z' should not be capitalized
func f2(IN, X int) (Y, Z int) { //<Loud:2,Capitalized:0,Capitalized:1,Capitalized:2
	return 0, 0
}

For that file, param-check emits these warnings (all matched):


testdata/param-name/checker_tests.go:5:9: param-name/Loud: consider `in' name instead of `IN'
testdata/param-name/checker_tests.go:5:18: param-name/Loud: consider `out' name instead of `OUT'
testdata/param-name/checker_tests.go:13:9: param-name/Loud: consider `in' name instead of `IN'
testdata/param-name/checker_tests.go:13:13: param-name/Capitalized: `X' should not be capitalized
testdata/param-name/checker_tests.go:13:21: param-name/Capitalized: `Y' should not be capitalized
testdata/param-name/checker_tests.go:13:24: param-name/Capitalized: `Z' should not be capitalized

It checks that:

  1. Warning is triggered.
  2. Warning line matches anchor line.
  3. Warning kind matches expected kind.
  4. Warning text matches expected text.

The benefits of this format:

  • Don't need to specify warning lines explicitly. They also aren't invalidated if file changes.
  • Anchors make it possible to place each warning matcher on it's own line (more readable than one 200+ chars line).
  • Order of matchers doesn't matter, so it's possible to run different kinds of sub-checks in separately or in parallel. In other words, using example above, param-check can emit Loud and Capitalized warnings in any order and output will still match. The content does matter, the order does not.

For every warning we want to match, there must be two comment directives inside test file:

  • Warning matcher
  • Warning anchor

Anchor is required to bind expected warning line,
and the matcher itself specifies expected warning kind and text.

Anchors and matchers connected with keys that consist of $kind:$id.
The $id must be unique integer constant.
You can't reuse ids even if error messages are the same because they need different line info.

Matcher looks like this:

//>Loud:0 consider `in' name instead of `IN'

Anchor for the matcher above:

//<Loud:0

Because one line can trigger multiple warnings, it's possible to specify more than one anchor (they may have different kinds):

//<Loud:2,Capitalized:0,Capitalized:1,Capitalized:2

@quasilyte
Copy link
Member Author

quasilyte commented May 9, 2018

@fexolm, @weeellz, @ludweeg what do you think?

@quasilyte
Copy link
Member Author

Now I think that requiring explicit ID's is dumb.
Trying to improve usability of this thing.

func f5(x, y, z interface{}) int {
	//>:8 case 0 can benefit from type switch with assignment
	switch x.(type) { //<:8
	case int:
		//>:9 case 0 can benefit from type switch with assignment
		switch y.(type) { //<:9
		case int:
			//>:10 case 0 can benefit from type switch with assignment
			switch z.(type) { //<:10
			case int:
				return x.(int) + y.(int) + z.(int)
			}
		}
	}
	return 0
}

When there are many warnings, it gets harder to assign proper ID.
And in 99.(9)% of cases we want simplest matching with anchors.

Maybe something like this is more bearable:

func f5(x, y, z interface{}) int {
	//>> case 0 can benefit from type switch with assignment
	switch x.(type) { //<<
	case int:
		//>> case 0 can benefit from type switch with assignment
		switch y.(type) { //<<
		case int:
			//>> case 0 can benefit from type switch with assignment
			switch z.(type) { //<<
			case int:
				return x.(int) + y.(int) + z.(int)
			}
		}
	}
	return 0
}

@quasilyte
Copy link
Member Author

Now I'm wondering if explicit //<< marker is needed.
We can assume that line after the //>> sequence is triggering line.

@fexolm
Copy link
Contributor

fexolm commented May 9, 2018

Anyway, we can improve functionality later. We need to test this first. I, actually, can not say a word without trying it myself.

@quasilyte
Copy link
Member Author

quasilyte commented May 9, 2018

@fexolm, OK.

I'm going to remove "anchors" completely.
//>> directives imply that following non-directive line is one that triggers one or more warnings.

@quasilyte
Copy link
Member Author

Almost there...

=== RUN   TestOutput
=== RUN   TestOutput/param-name
=== RUN   TestOutput/type-guard
=== RUN   TestOutput/parenthesis
=== RUN   TestOutput/param-duplication
=== RUN   TestOutput/underef
--- PASS: TestOutput (0.01s)
    --- PASS: TestOutput/param-name (0.00s)
    --- PASS: TestOutput/type-guard (0.00s)
    --- PASS: TestOutput/parenthesis (0.00s)
    --- PASS: TestOutput/param-duplication (0.00s)
    --- PASS: TestOutput/underef (0.00s)
PASS
ok  	github.com/PieselBois/kfulint/cmd/kfulint	0.475s

quasilyte added a commit that referenced this issue May 9, 2018
Ported all existing testdata files to new format.

The format is simple:
Use "///: blah-blah" or "///kind: blah-blah" comments
to mark line that comes after a sequence of "///" comments
as warning triggering line.

Warning triggering line is expected to make linter
emit every message enumerated among these directives.

See changed test files for examples.

Updates #8
quasilyte pushed a commit that referenced this issue May 10, 2018
Ported all existing testdata files to new format.

The format is simple:
Use "///: blah-blah" or "///kind: blah-blah" comments
to mark line that comes after a sequence of "///" comments
as warning triggering line.

Warning triggering line is expected to make linter
emit every message enumerated among these directives.

See changed test files for examples.

Updates #8
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

No branches or pull requests

2 participants