From 331a1c68965eaef2ef44e7d94037cd4d8a7ea346 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 12 Jan 2023 18:12:26 -0500 Subject: [PATCH] gopls/internal/regtest: add a simpler API for diagnostic expectations The number of different regtest expectations related to diagnostics has grown significantly over time. Start to consolidate to just two expectations: Diagnostics, which asserts on the existence of diagnostics, and NoMatchingDiagnostics, which asserts on the nonexistence of diagnostics. Both accept an argument list to filter the set of diagnostics under consideration. In a subsequent CL, NoMatchingDiagnostics will be renamed to NoDiagnostics, once the existing expectation with that name has been replaced. Use this to eliminate the following expectations: - DiagnosticAtRegexpFromSource -> Diagnostics(AtRegexp, FromSource) - NoDiagnosticAtRegexp -> NoMatchingDiagnostics(AtRegexp) - NoOutstandingDiagnostics -> NoMatchingDiagnostics Updates golang/go#39384 Change-Id: I518b14eb00c9fcf62388a03efb668899939a8f82 Reviewed-on: https://go-review.googlesource.com/c/tools/+/461915 Run-TryBot: Robert Findley Reviewed-by: Alan Donovan gopls-CI: kokoro TryBot-Result: Gopher Robot --- gopls/internal/lsp/regtest/expectation.go | 163 ++++++++++++++---- .../regtest/codelens/codelens_test.go | 2 +- .../regtest/diagnostics/diagnostics_test.go | 2 +- .../internal/regtest/misc/staticcheck_test.go | 21 +-- .../internal/regtest/workspace/broken_test.go | 9 +- .../regtest/workspace/standalone_test.go | 42 +---- .../regtest/workspace/workspace_test.go | 6 +- 7 files changed, 152 insertions(+), 93 deletions(-) diff --git a/gopls/internal/lsp/regtest/expectation.go b/gopls/internal/lsp/regtest/expectation.go index 37113861204..a86dfe10219 100644 --- a/gopls/internal/lsp/regtest/expectation.go +++ b/gopls/internal/lsp/regtest/expectation.go @@ -700,23 +700,144 @@ func (e DiagnosticExpectation) Description() string { return desc } -// NoOutstandingDiagnostics asserts that the workspace has no outstanding -// diagnostic messages. -func NoOutstandingDiagnostics() Expectation { +// Diagnostics asserts that there is at least one diagnostic matching the given +// filters. +func Diagnostics(filters ...DiagnosticFilter) Expectation { check := func(s State) Verdict { - for _, diags := range s.diagnostics { - if len(diags.Diagnostics) > 0 { + diags := flattenDiagnostics(s) + for _, filter := range filters { + var filtered []flatDiagnostic + for _, d := range diags { + if filter.check(d.name, d.diag) { + filtered = append(filtered, d) + } + } + if len(filtered) == 0 { + // TODO(rfindley): if/when expectations describe their own failure, we + // can provide more useful information here as to which filter caused + // the failure. return Unmet } + diags = filtered + } + return Met + } + var descs []string + for _, filter := range filters { + descs = append(descs, filter.desc) + } + return SimpleExpectation{ + check: check, + description: "any diagnostics " + strings.Join(descs, ", "), + } +} + +// NoMatchingDiagnostics asserts that there are no diagnostics matching the +// given filters. Notably, if no filters are supplied this assertion checks +// that there are no diagnostics at all, for any file. +// +// TODO(rfindley): replace NoDiagnostics with this, and rename. +func NoMatchingDiagnostics(filters ...DiagnosticFilter) Expectation { + check := func(s State) Verdict { + diags := flattenDiagnostics(s) + for _, filter := range filters { + var filtered []flatDiagnostic + for _, d := range diags { + if filter.check(d.name, d.diag) { + filtered = append(filtered, d) + } + } + diags = filtered + } + if len(diags) > 0 { + return Unmet } return Met } + var descs []string + for _, filter := range filters { + descs = append(descs, filter.desc) + } return SimpleExpectation{ check: check, - description: "no outstanding diagnostics", + description: "no diagnostics " + strings.Join(descs, ", "), + } +} + +type flatDiagnostic struct { + name string + diag protocol.Diagnostic +} + +func flattenDiagnostics(state State) []flatDiagnostic { + var result []flatDiagnostic + for name, diags := range state.diagnostics { + for _, diag := range diags.Diagnostics { + result = append(result, flatDiagnostic{name, diag}) + } } + return result } +// -- Diagnostic filters -- + +// A DiagnosticFilter filters the set of diagnostics, for assertion with +// Diagnostics or NoMatchingDiagnostics. +type DiagnosticFilter struct { + desc string + check func(name string, _ protocol.Diagnostic) bool +} + +// ForFile filters to diagnostics matching the sandbox-relative file name. +func ForFile(name string) DiagnosticFilter { + return DiagnosticFilter{ + desc: fmt.Sprintf("for file %q", name), + check: func(diagName string, _ protocol.Diagnostic) bool { + return diagName == name + }, + } +} + +// FromSource filters to diagnostics matching the given diagnostics source. +func FromSource(source string) DiagnosticFilter { + return DiagnosticFilter{ + desc: fmt.Sprintf("with source %q", source), + check: func(_ string, d protocol.Diagnostic) bool { + return d.Source == source + }, + } +} + +// AtRegexp filters to diagnostics in the file with sandbox-relative path name, +// at the first position matching the given regexp pattern. +// +// TODO(rfindley): pass in the editor to expectations, so that they may depend +// on editor state and AtRegexp can be a function rather than a method. +func (e *Env) AtRegexp(name, pattern string) DiagnosticFilter { + pos := e.RegexpSearch(name, pattern) + return DiagnosticFilter{ + desc: fmt.Sprintf("at the first position matching %q in %q", pattern, name), + check: func(diagName string, d protocol.Diagnostic) bool { + // TODO(rfindley): just use protocol.Position for Pos, rather than + // duplicating. + return diagName == name && d.Range.Start.Line == uint32(pos.Line) && d.Range.Start.Character == uint32(pos.Column) + }, + } +} + +// WithMessageContaining filters to diagnostics whose message contains the +// given substring. +func WithMessageContaining(substring string) DiagnosticFilter { + return DiagnosticFilter{ + desc: fmt.Sprintf("with message containing %q", substring), + check: func(_ string, d protocol.Diagnostic) bool { + return strings.Contains(d.Message, substring) + }, + } +} + +// TODO(rfindley): eliminate all expectations below this point. + // NoDiagnostics asserts that either no diagnostics are sent for the // workspace-relative path name, or empty diagnostics are sent. func NoDiagnostics(name string) Expectation { @@ -749,43 +870,17 @@ func (e *Env) DiagnosticAtRegexpWithMessage(name, re, msg string) DiagnosticExpe return DiagnosticExpectation{path: name, pos: &pos, re: re, present: true, message: msg} } -// DiagnosticAtRegexpFromSource expects a diagnostic at the first position -// matching re, from the given source. -func (e *Env) DiagnosticAtRegexpFromSource(name, re, source string) DiagnosticExpectation { - e.T.Helper() - pos := e.RegexpSearch(name, re) - return DiagnosticExpectation{path: name, pos: &pos, re: re, present: true, source: source} -} - // DiagnosticAt asserts that there is a diagnostic entry at the position // specified by line and col, for the workdir-relative path name. func DiagnosticAt(name string, line, col int) DiagnosticExpectation { return DiagnosticExpectation{path: name, pos: &fake.Pos{Line: line, Column: col}, present: true} } -// NoDiagnosticAtRegexp expects that there is no diagnostic entry at the start -// position matching the regexp search string re in the buffer specified by -// name. Note that this currently ignores the end position. -// This should only be used in combination with OnceMet for a given condition, -// otherwise it may always succeed. -func (e *Env) NoDiagnosticAtRegexp(name, re string) DiagnosticExpectation { - e.T.Helper() - pos := e.RegexpSearch(name, re) - return DiagnosticExpectation{path: name, pos: &pos, re: re, present: false} -} - -// NoDiagnosticWithMessage asserts that there is no diagnostic entry with the -// given message. -// -// This should only be used in combination with OnceMet for a given condition, -// otherwise it may always succeed. -func NoDiagnosticWithMessage(name, msg string) DiagnosticExpectation { - return DiagnosticExpectation{path: name, message: msg, present: false} -} - // GoSumDiagnostic asserts that a "go.sum is out of sync" diagnostic for the // given module (as formatted in a go.mod file, e.g. "example.com v1.0.0") is // present. +// +// TODO(rfindley): remove this. func (e *Env) GoSumDiagnostic(name, module string) Expectation { e.T.Helper() // In 1.16, go.sum diagnostics should appear on the relevant module. Earlier diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go index 8ea5e9c6fcf..13ba4e1c305 100644 --- a/gopls/internal/regtest/codelens/codelens_test.go +++ b/gopls/internal/regtest/codelens/codelens_test.go @@ -216,7 +216,7 @@ require golang.org/x/hello v1.2.3 // but there may be some subtlety in timing here, where this // should always succeed, but may not actually test the correct // behavior. - env.NoDiagnosticAtRegexp("b/go.mod", `require`), + NoMatchingDiagnostics(env.AtRegexp("b/go.mod", `require`)), ), ) // Check for upgrades in b/go.mod and then clear them. diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index 6631a9234ca..b7ccb5bf8d4 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -1404,7 +1404,7 @@ func main() { env.Await( OnceMet( InitialWorkspaceLoad, - NoDiagnosticWithMessage("", "illegal character U+0023 '#'"), + NoMatchingDiagnostics(WithMessageContaining("illegal character U+0023 '#'")), ), ) }) diff --git a/gopls/internal/regtest/misc/staticcheck_test.go b/gopls/internal/regtest/misc/staticcheck_test.go index f2ba3ccd8a7..9242e194eb7 100644 --- a/gopls/internal/regtest/misc/staticcheck_test.go +++ b/gopls/internal/regtest/misc/staticcheck_test.go @@ -64,13 +64,13 @@ var FooErr error = errors.New("foo") Settings{"staticcheck": true}, ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("a/a.go") - env.Await( - env.DiagnosticAtRegexpFromSource("a/a.go", "sort.Slice", "sortslice"), - env.DiagnosticAtRegexpFromSource("a/a.go", "sort.Slice.(slice)", "SA1028"), - env.DiagnosticAtRegexpFromSource("a/a.go", "var (FooErr)", "ST1012"), - env.DiagnosticAtRegexpFromSource("a/a.go", `"12234"`, "SA1024"), - env.DiagnosticAtRegexpFromSource("a/a.go", "testGenerics.*(p P)", "SA4009"), - env.DiagnosticAtRegexpFromSource("a/a.go", "q = (&\\*p)", "SA4001"), + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "sort.Slice"), FromSource("sortslice")), + Diagnostics(env.AtRegexp("a/a.go", "sort.Slice.(slice)"), FromSource("SA1028")), + Diagnostics(env.AtRegexp("a/a.go", "var (FooErr)"), FromSource("ST1012")), + Diagnostics(env.AtRegexp("a/a.go", `"12234"`), FromSource("SA1024")), + Diagnostics(env.AtRegexp("a/a.go", "testGenerics.*(p P)"), FromSource("SA4009")), + Diagnostics(env.AtRegexp("a/a.go", "q = (&\\*p)"), FromSource("SA4001")), ) }) } @@ -103,11 +103,8 @@ func Foo(enabled interface{}) { Settings{"staticcheck": true}, ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("p.go") - env.Await( - OnceMet( - env.DoneWithOpen(), - env.DiagnosticAtRegexpFromSource("p.go", ", (enabled)", "SA9008"), - ), + env.AfterChange( + Diagnostics(env.AtRegexp("p.go", ", (enabled)"), FromSource("SA9008")), ) }) } diff --git a/gopls/internal/regtest/workspace/broken_test.go b/gopls/internal/regtest/workspace/broken_test.go index 711d59617be..9a65030c715 100644 --- a/gopls/internal/regtest/workspace/broken_test.go +++ b/gopls/internal/regtest/workspace/broken_test.go @@ -160,16 +160,13 @@ const F = named.D - 3 Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("p/internal/bar/bar.go") - env.Await( - OnceMet( - env.DoneWithOpen(), - env.DiagnosticAtRegexp("p/internal/bar/bar.go", "\"mod.test/p/internal/foo\""), - ), + env.AfterChange( + env.DiagnosticAtRegexp("p/internal/bar/bar.go", "\"mod.test/p/internal/foo\""), ) env.OpenFile("go.mod") env.RegexpReplace("go.mod", "mod.testx", "mod.test") env.SaveBuffer("go.mod") // saving triggers a reload - env.Await(NoOutstandingDiagnostics()) + env.AfterChange(NoMatchingDiagnostics()) }) } diff --git a/gopls/internal/regtest/workspace/standalone_test.go b/gopls/internal/regtest/workspace/standalone_test.go index 698c8aac134..dffbbea4b70 100644 --- a/gopls/internal/regtest/workspace/standalone_test.go +++ b/gopls/internal/regtest/workspace/standalone_test.go @@ -74,29 +74,14 @@ func main() { } env.OpenFile("lib/lib.go") - env.Await( - OnceMet( - env.DoneWithOpen(), - NoOutstandingDiagnostics(), - ), - ) + env.AfterChange(NoMatchingDiagnostics()) // Replacing C with D should not cause any workspace diagnostics, since we // haven't yet opened the standalone file. env.RegexpReplace("lib/lib.go", "C", "D") - env.Await( - OnceMet( - env.DoneWithChange(), - NoOutstandingDiagnostics(), - ), - ) + env.AfterChange(NoMatchingDiagnostics()) env.RegexpReplace("lib/lib.go", "D", "C") - env.Await( - OnceMet( - env.DoneWithChange(), - NoOutstandingDiagnostics(), - ), - ) + env.AfterChange(NoMatchingDiagnostics()) refs := env.References("lib/lib.go", env.RegexpSearch("lib/lib.go", "C")) checkLocations("References", refs, "lib/lib.go") @@ -106,12 +91,7 @@ func main() { // Opening the standalone file should not result in any diagnostics. env.OpenFile("lib/ignore.go") - env.Await( - OnceMet( - env.DoneWithOpen(), - NoOutstandingDiagnostics(), - ), - ) + env.AfterChange(NoMatchingDiagnostics()) // Having opened the standalone file, we should find its symbols in the // workspace. @@ -151,21 +131,11 @@ func main() { // Renaming "lib.C" to "lib.D" should cause a diagnostic in the standalone // file. env.RegexpReplace("lib/lib.go", "C", "D") - env.Await( - OnceMet( - env.DoneWithChange(), - env.DiagnosticAtRegexp("lib/ignore.go", "lib.(C)"), - ), - ) + env.AfterChange(env.DiagnosticAtRegexp("lib/ignore.go", "lib.(C)")) // Undoing the replacement should fix diagnostics env.RegexpReplace("lib/lib.go", "D", "C") - env.Await( - OnceMet( - env.DoneWithChange(), - NoOutstandingDiagnostics(), - ), - ) + env.AfterChange(NoMatchingDiagnostics()) // Now that our workspace has no errors, we should be able to find // references and rename. diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index a279b8e2a6b..49e9e27226c 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/gopls/internal/regtest/workspace/workspace_test.go @@ -272,7 +272,7 @@ func Hello() int { env.AfterChange( env.DiagnosticAtRegexp("moda/a/a.go", "x"), env.DiagnosticAtRegexp("modb/b/b.go", "x"), - env.NoDiagnosticAtRegexp("moda/a/a.go", `"b.com/b"`), + NoMatchingDiagnostics(env.AtRegexp("moda/a/a.go", `"b.com/b"`)), ) }) } @@ -702,7 +702,7 @@ module example.com/bar // the diagnostic still shows up. env.SetBufferContent("go.work", "go 1.18 \n\n use ./bar\n") env.AfterChange( - env.NoDiagnosticAtRegexp("go.work", "use"), + NoMatchingDiagnostics(env.AtRegexp("go.work", "use")), ) env.SetBufferContent("go.work", "go 1.18 \n\n use ./foo\n") env.AfterChange( @@ -1069,7 +1069,7 @@ use ( b ) `) - env.AfterChange(NoOutstandingDiagnostics()) + env.AfterChange(NoMatchingDiagnostics()) // Removing the go.work file should put us back where we started. env.RemoveWorkspaceFile("go.work")