lsp: Address test concurrency issues #1112

charlieegan3 · 2024-09-16T14:12:55Z

This PR is intended to address two issues with the test suite that happen when running tests concurrently:

TestLanguageServerSingleFile fails sometimes as the server logs an error when the context has been cancelled, and it's a panic when logging to t, after the test is over.

https://github.com/StyraInc/regal/actions/runs/10875806798/job/30174833322

TestGetInlayHintsAstTerms will fail when TestLanguageServerSingleFile is running sometimes as TestLanguageServerSingleFile updates the contents of the map while TestGetInlayHintsAstTerms is iterating over it.

https://github.com/StyraInc/regal/actions/runs/10875806798/job/30184826633

Coincidentally, both these issues happened on a recent community PR, which underlines the importance of addressing these as they're a bad contributor experience.

This PR also includes, in a second commit 554f612, an update to rego.QueryRegalBundle to accept a context. See commit message, but there appears to be a recurring issue here where tests timeout.

IO have updated the function an callers to pass a context to this function to allow us to better understand this issue.

charlieegan3 · 2024-09-16T14:13:14Z

internal/lsp/rego/builtins.go

@@ -23,7 +24,7 @@ func GetBuiltins() map[string]*ast.Builtin {
 	builtInsLock.RLock()
 	defer builtInsLock.RUnlock()

-	return builtIns
+	return maps.Clone(builtIns)


Similar to what we have in the cache, we return a copy instead.

charlieegan3 · 2024-09-16T14:13:41Z

internal/lsp/rego/rego.go

@@ -56,6 +56,8 @@ func LocationFromPosition(pos types.Position) *ast.Location {
 func AllBuiltinCalls(module *ast.Module) []BuiltInCall {
 	builtinCalls := make([]BuiltInCall, 0)

+	bis := GetBuiltins()


Loading the builtins once also seemed like a good plan here since we're now loading a copy.

charlieegan3 · 2024-09-16T14:14:20Z

internal/lsp/server_test.go

+		tl.mu.Lock()
+		defer tl.mu.Unlock()
+		tl.open = false
+	})


When the test is over, the logger is closed to stop it accepting more logs and causing a panic when the test is done.

This PR is intended to address two issues with the test suite that happen when running tests concurrently: TestLanguageServerSingleFile fails sometimes as the server logs an error when the context has been cancelled, and it's a panic when logging to t, after the test is over. https://github.com/StyraInc/regal/actions/runs/10875806798/job/30174833322 TestGetInlayHintsAstTerms will fail when TestLanguageServerSingleFile is running sometimes as TestLanguageServerSingleFile updates the contents of the map while TestGetInlayHintsAstTerms is iterating over it. https://github.com/StyraInc/regal/actions/runs/10875806798/job/30184826633 Signed-off-by: Charlie Egan <[email protected]>

There are cases where the tests timeout running `rego.QueryRegalBundle`: Here are two from today: https://github.com/StyraInc/regal/actions/runs/10883075294/job/30195419440 https://github.com/StyraInc/regal/actions/runs/10883075932/job/30195421353 My hope is that we can use a cancelled context to avoid this and see what the real issue is, if there is one. Signed-off-by: Charlie Egan <[email protected]>

srenatus · 2024-09-16T17:16:04Z

internal/lsp/completions/providers/builtins_test.go

@@ -5,6 +5,8 @@ import (
 	"strings"
 	"testing"

+	"golang.org/x/net/context"


This shouldn't be used anymore, probably an auto import

oops, thanks, have added a new commit to address

Signed-off-by: Charlie Egan <[email protected]>

srenatus

LGTM

charlieegan3 · 2024-09-17T08:54:18Z

Thanks Stephan, appreciate the review here 🙏

Built ins must now be provided by the caller. We have been having a number of issues where the shared list of builtins was required to be in a different state in different tests. Related StyraInc#1101 StyraInc#1102 StyraInc#1112 StyraInc#1121 StyraInc#1129 Signed-off-by: Charlie Egan <[email protected]>

Built ins must now be provided by the caller. We have been having a number of issues where the shared list of builtins was required to be in a different state in different tests. Related #1101 #1102 #1112 #1121 #1129 Signed-off-by: Charlie Egan <[email protected]>

* lsp: Address test concurrency issues This PR is intended to address two issues with the test suite that happen when running tests concurrently: TestLanguageServerSingleFile fails sometimes as the server logs an error when the context has been cancelled, and it's a panic when logging to t, after the test is over. https://github.com/StyraInc/regal/actions/runs/10875806798/job/30174833322 TestGetInlayHintsAstTerms will fail when TestLanguageServerSingleFile is running sometimes as TestLanguageServerSingleFile updates the contents of the map while TestGetInlayHintsAstTerms is iterating over it. https://github.com/StyraInc/regal/actions/runs/10875806798/job/30184826633 Signed-off-by: Charlie Egan <[email protected]> * lsp: Pass ctx when querying the Regal bundle There are cases where the tests timeout running `rego.QueryRegalBundle`: Here are two from today: https://github.com/StyraInc/regal/actions/runs/10883075294/job/30195419440 https://github.com/StyraInc/regal/actions/runs/10883075932/job/30195421353 My hope is that we can use a cancelled context to avoid this and see what the real issue is, if there is one. Signed-off-by: Charlie Egan <[email protected]> * tests: Drop old context import Signed-off-by: Charlie Egan <[email protected]> --------- Signed-off-by: Charlie Egan <[email protected]>

Built ins must now be provided by the caller. We have been having a number of issues where the shared list of builtins was required to be in a different state in different tests. Related StyraInc#1101 StyraInc#1102 StyraInc#1112 StyraInc#1121 StyraInc#1129 Signed-off-by: Charlie Egan <[email protected]>

* lsp: Address test concurrency issues This PR is intended to address two issues with the test suite that happen when running tests concurrently: TestLanguageServerSingleFile fails sometimes as the server logs an error when the context has been cancelled, and it's a panic when logging to t, after the test is over. https://github.com/StyraInc/regal/actions/runs/10875806798/job/30174833322 TestGetInlayHintsAstTerms will fail when TestLanguageServerSingleFile is running sometimes as TestLanguageServerSingleFile updates the contents of the map while TestGetInlayHintsAstTerms is iterating over it. https://github.com/StyraInc/regal/actions/runs/10875806798/job/30184826633 Signed-off-by: Charlie Egan <[email protected]> * lsp: Pass ctx when querying the Regal bundle There are cases where the tests timeout running `rego.QueryRegalBundle`: Here are two from today: https://github.com/StyraInc/regal/actions/runs/10883075294/job/30195419440 https://github.com/StyraInc/regal/actions/runs/10883075932/job/30195421353 My hope is that we can use a cancelled context to avoid this and see what the real issue is, if there is one. Signed-off-by: Charlie Egan <[email protected]> * tests: Drop old context import Signed-off-by: Charlie Egan <[email protected]> --------- Signed-off-by: Charlie Egan <[email protected]>

Built ins must now be provided by the caller. We have been having a number of issues where the shared list of builtins was required to be in a different state in different tests. Related StyraInc#1101 StyraInc#1102 StyraInc#1112 StyraInc#1121 StyraInc#1129 Signed-off-by: Charlie Egan <[email protected]>

charlieegan3 commented Sep 16, 2024

View reviewed changes

charlieegan3 force-pushed the address-test-issues branch 2 times, most recently from edcc758 to af6fa23 Compare September 16, 2024 14:54

charlieegan3 added 2 commits September 16, 2024 18:29

charlieegan3 force-pushed the address-test-issues branch from af6fa23 to 6a46f50 Compare September 16, 2024 16:29

srenatus reviewed Sep 16, 2024

View reviewed changes

tests: Drop old context import

4b406ff

Signed-off-by: Charlie Egan <[email protected]>

srenatus approved these changes Sep 16, 2024

View reviewed changes

charlieegan3 merged commit 06734bb into StyraInc:main Sep 17, 2024
4 checks passed

charlieegan3 mentioned this pull request Sep 24, 2024

tests: Remove global builtins state #1134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lsp: Address test concurrency issues #1112

lsp: Address test concurrency issues #1112

charlieegan3 commented Sep 16, 2024 •

edited

Loading

charlieegan3 Sep 16, 2024

charlieegan3 Sep 16, 2024

charlieegan3 Sep 16, 2024

srenatus Sep 16, 2024

charlieegan3 Sep 16, 2024

srenatus left a comment

charlieegan3 commented Sep 17, 2024

lsp: Address test concurrency issues #1112

lsp: Address test concurrency issues #1112

Conversation

charlieegan3 commented Sep 16, 2024 • edited Loading

charlieegan3 Sep 16, 2024

Choose a reason for hiding this comment

charlieegan3 Sep 16, 2024

Choose a reason for hiding this comment

charlieegan3 Sep 16, 2024

Choose a reason for hiding this comment

srenatus Sep 16, 2024

Choose a reason for hiding this comment

charlieegan3 Sep 16, 2024

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

charlieegan3 commented Sep 17, 2024

charlieegan3 commented Sep 16, 2024 •

edited

Loading