-
Notifications
You must be signed in to change notification settings - Fork 868
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
Migrate temporalite & temporaltest packages #4026
Merged
yiminc
merged 56 commits into
temporalio:main
from
jlegrone:jlegrone/migrate-temporalite
Oct 18, 2023
Merged
Changes from 26 commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
804337e
Add temporalite & temporaltest packages
jlegrone ceb5741
Move test workflow/interceptor into temoraltest package
jlegrone 9062c19
Move liteconfig into temporalite package
jlegrone 9a527fd
Remove ui server
jlegrone b8fee47
Remove unused timeoutFromContext func
jlegrone 53d73e1
Avoid stuttering in temporalite package
jlegrone b5e7487
Always use dynamic ports
jlegrone 3f2d1e5
Add additional config doc comments
jlegrone 73dacd0
Update copyright
jlegrone 2ca1af6
Refactor temporalite into temporal package
jlegrone 1022888
Remove PublicClient config
jlegrone e650816
Revert "Remove PublicClient config"
jlegrone 34760eb
Add license header
jlegrone 9faaebc
Test setting BaseServerOptions
jlegrone c613ba8
Update godoc
jlegrone 0cd959a
Move temporaltest to top level
jlegrone ecd00c4
Update temporaltest backwards compatability policy
jlegrone 2e4bd13
Spellcheck
jlegrone 40fd094
Update godoc
jlegrone 73d31cc
Add temporaltest package to integration tests
jlegrone c512a54
Move LiteServer into internal package
jlegrone 9ecf117
Merge branch 'master' into jlegrone/migrate-temporalite
jlegrone 87a65ed
Add TODO to remove PublicClient config
jlegrone 43810ea
Document that TestServer methods are unsafe for concurrent use
jlegrone 6aa60b0
Don't return an error from denyAllClaimMapper
jlegrone 2b717b0
Merge branch 'master' into jlegrone/migrate-temporalite
jlegrone b41ac72
Merge branch 'master' into jlegrone/migrate-temporalite
jlegrone 3030a0c
Test registering and listing workflows using custom search attribute
jlegrone f8ff2a7
Use errors.As to check for permission denied
jlegrone 04bf716
Verify that a custom worker option was set
jlegrone f9d60cf
Add TODO to investigate whether randomized db name is necessary in sh…
jlegrone 5a39dec
Return error instead of panic while getting port
jlegrone ac773d3
Update freeport.go to be unique implementation
jlegrone ff3ebf3
Remove temporaltest from UNIT_TEST_DIRS
jlegrone 8d29d27
Skip conditional check when appending opts
jlegrone cf56460
Remove godoc comment about overriding ConnectionOptions
jlegrone 9a0e082
Use maps.Keys
jlegrone 4d1d04b
Don't set MembershipPort twice
jlegrone 4f4070c
Add log.Logger type assertion
jlegrone d240ccf
Simplify TestServerOption applyFunc
jlegrone 4ecfb55
Delegate to NewWorkerWithOptions
jlegrone 0da8117
Start server synchronously
jlegrone 12bdf7a
Add internal/temporalite to integration tests
jlegrone f599ac3
Merge branch 'main' into jlegrone/migrate-temporalite
jlegrone d233bed
Flaky fixes
jlegrone 364b2db
Remove deprecated rand.Seed call
jlegrone af4f963
WIP
jlegrone 67032f0
Use Eventually to speed up TestSearchAttributeRegistration
jlegrone 20ab82a
Merge branch 'main' into jlegrone/migrate-temporalite
jlegrone fe318f9
Merge branch 'main' into jlegrone/migrate-temporalite
yiminc 60c302e
Merge branch 'main' into jlegrone/migrate-temporalite
jlegrone b5e2ef7
Merge branch 'main' into jlegrone/migrate-temporalite
yiminc 211c9af
Fix lint warnings: add error handling on test server shutdown
jlegrone 5d217dd
Inline useless else branch
jlegrone a936d52
Fix unit test filter
jlegrone 79883a7
Remove parallelism from temporaltest tests to avoid tripping race det…
jlegrone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,7 @@ FUNCTIONAL_TEST_XDC_ROOT := ./tests/xdc | |
FUNCTIONAL_TEST_NDC_ROOT := ./tests/ndc | ||
DB_INTEGRATION_TEST_ROOT := ./common/persistence/tests | ||
DB_TOOL_INTEGRATION_TEST_ROOT := ./tools/tests | ||
INTEGRATION_TEST_DIRS := $(DB_INTEGRATION_TEST_ROOT) $(DB_TOOL_INTEGRATION_TEST_ROOT) | ||
INTEGRATION_TEST_DIRS := $(DB_INTEGRATION_TEST_ROOT) $(DB_TOOL_INTEGRATION_TEST_ROOT) ./temporaltest | ||
UNIT_TEST_DIRS := $(filter-out $(FUNCTIONAL_TEST_ROOT)% $(FUNCTIONAL_TEST_XDC_ROOT)% $(FUNCTIONAL_TEST_NDC_ROOT)% $(DB_INTEGRATION_TEST_ROOT)% $(DB_TOOL_INTEGRATION_TEST_ROOT)%,$(TEST_DIRS)) | ||
|
||
# github.com/urfave/cli/[email protected] - needs to accept comma in values before unlocking https://github.com/urfave/cli/pull/1241. | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
// The MIT License | ||
// | ||
// Copyright (c) 2021 Datadog, Inc. | ||
// | ||
// Copyright (c) 2020 Temporal Technologies Inc. All rights reserved. | ||
// | ||
// Copyright (c) 2020 Uber Technologies, Inc. | ||
// | ||
// Permission is hereby granted, free of charge, to any person obtaining a copy | ||
// of this software and associated documentation files (the "Software"), to deal | ||
// in the Software without restriction, including without limitation the rights | ||
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
// copies of the Software, and to permit persons to whom the Software is | ||
// furnished to do so, subject to the following conditions: | ||
// | ||
// The above copyright notice and this permission notice shall be included in | ||
// all copies or substantial portions of the Software. | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
// THE SOFTWARE. | ||
|
||
package temporalite | ||
|
||
import ( | ||
"fmt" | ||
"net" | ||
) | ||
|
||
// Modified from https://github.com/phayes/freeport/blob/95f893ade6f232a5f1511d61735d89b1ae2df543/freeport.go | ||
jlegrone marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
func newPortProvider() *portProvider { | ||
return &portProvider{} | ||
} | ||
|
||
type portProvider struct { | ||
listeners []*net.TCPListener | ||
} | ||
|
||
// GetFreePort asks the kernel for a free open port that is ready to use. | ||
func (p *portProvider) GetFreePort() (int, error) { | ||
addr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:0") | ||
if err != nil { | ||
if addr, err = net.ResolveTCPAddr("tcp6", "[::1]:0"); err != nil { | ||
panic(fmt.Sprintf("temporalite: failed to get free port: %v", err)) | ||
jlegrone marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
l, err := net.ListenTCP("tcp", addr) | ||
if err != nil { | ||
return 0, err | ||
} | ||
|
||
p.listeners = append(p.listeners, l) | ||
|
||
return l.Addr().(*net.TCPAddr).Port, nil | ||
} | ||
|
||
func (p *portProvider) MustGetFreePort() int { | ||
port, err := p.GetFreePort() | ||
if err != nil { | ||
panic(err) | ||
} | ||
return port | ||
} | ||
|
||
func (p *portProvider) Close() error { | ||
for _, l := range p.listeners { | ||
if err := l.Close(); err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Right now
temporaltest
is being run with both unit and integration test targets. In CI it consistently passes in unit tests, but fails in integration tests.I can't reproduce this failure locally either with
go test ./temporaltest
ormake integration-test-coverage
(the latter after removingDB_INTEGRATION_TEST_ROOT
andDB_TOOL_INTEGRATION_TEST_ROOT
on this line since I don't have DBs running).Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I'm able to reproduce the integration test failures locally after starting up dependencies using
make start-dependencies
. Theintegration-test
target succeeds:But coverage fails:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I've gotten three different failures through
make unit-test
locally as well (after running it in a loop a few hundred times):Client must be created with client.Dial() or client.NewLazyClient()
nil pointer exception
data race
Perhaps we should quarantine
temporaltest
in an internal package for now until we know it can be used reliably?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, bugs when you stop the server immediately after starting or before it's fully started are not surprising. I did a little fx refactoring to try to avoid some of them so if you merge with the latest changes I think those should be reduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd set up the test targets so that these run in "integration" tests and not unit or functional. Probably all of
./temporal
should be there too.