Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Address races in test cases #1102

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Conversation

charlieegan3
Copy link
Member

@charlieegan3 charlieegan3 commented Sep 11, 2024

There were three areas/issues:

  • An error var in a config watcher test was accessed in two places
  • server loaded config was accessed in a number of places without locking
  • a panic caused by early closing of the net.Pipe in the lsp server tests.

This PR addresses these issues and runs the race check on PRs during the Linux build.

Fixes #1099

Signed-off-by: Charlie Egan <charlie@styra.com>
This was causing errors from one of our new tests, but would be the same
for accessing the config in the various workers we have too.

Signed-off-by: Charlie Egan <charlie@styra.com>
@charlieegan3 charlieegan3 marked this pull request as draft September 11, 2024 14:46
@charlieegan3 charlieegan3 marked this pull request as ready for review September 11, 2024 14:56
This was causing a race condition where the net pipe connections were
closed before the ctx was cancelled closing the jsonrpc connections.

Signed-off-by: Charlie Egan <charlie@styra.com>
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a chore! Well done 👏

@@ -56,6 +56,8 @@ jobs:
path: ~/go/bin/rq
key: ${{ runner.os }}-${{ runner.arch }}-go-rq-${{ env.RQ_VERSION }}
- run: build/do.rq pull_request
- run: go test -race ./...
if: matrix.os.name == 'linux'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is tolerable since the intel macos build is going to be slower anyway 😄 But when we get rid of that, we should run this in a parallel job.

@anderseknert anderseknert merged commit d754094 into StyraInc:main Sep 11, 2024
4 checks passed
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Sep 24, 2024
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 <charlie@styra.com>
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Sep 24, 2024
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 <charlie@styra.com>
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Sep 24, 2024
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 <charlie@styra.com>
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Sep 24, 2024
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 <charlie@styra.com>
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Sep 24, 2024
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 <charlie@styra.com>
anderseknert pushed a commit that referenced this pull request Sep 24, 2024
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 <charlie@styra.com>
srenatus pushed a commit to srenatus/regal that referenced this pull request Oct 1, 2024
And add `go test -race` to CI

Charlie Egan <charlie@styra.com>
srenatus pushed a commit to srenatus/regal that referenced this pull request Oct 1, 2024
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 <charlie@styra.com>
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Jan 6, 2025
And add `go test -race` to CI

Charlie Egan <charlie@styra.com>
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Jan 6, 2025
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 <charlie@styra.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix go test -race ./...issues and run in CI
2 participants