-
Notifications
You must be signed in to change notification settings - Fork 116
add support for LOCAL_LAMBDA_PORT #557
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
Conversation
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.
One minor issue in the tests. I think you are going to have to group together all the tests that use LambdaRuntime and mark them with the .serialized
test trait so they don't run on parallel
|
||
// Set environment variable | ||
setenv("LOCAL_LAMBDA_PORT", "\(customPort)", 1) | ||
defer { unsetenv("LOCAL_LAMBDA_PORT") } |
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.
Given these tests are running in parallel, there is a chance the LOCAL_LAMBDA_PORT environment var setting will affect other tests.
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.
@adam-fowler good point. I aggressively added .serialized
on all tests using the server or the runtime client
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 that might only run tests serially within each suite so two suites can still run in parallel, even though the tests internally run serially. Might be worth asking swift-testing folks
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.
@adam-fowler During the tests, we let the MockServer bind on any available port (port = 0) and share its actual port number with the runtime client. There is no risk of conflict.
See
func withMockServer<Result>( |
I kept the .serialized
only on the new test in case someone adds another test to this suite later on.
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.
The problem isn't the fact you might have multiple servers binding to the same address (although that is a problem and am surprised it hasn't reared it head before). The problem is the environment variable is being set when other tests could be running and could affect those tests.
We can leave as is, but I think we will get the occasional test failure as multiple tests inherit the environment variable and bind to the same address.
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.
am surprised it hasn't reared it head before
In general, the unit tests don't use the local server. They use a MockServer that uses a random port number to avoid conflicts.
I think we will get the occasional test failure as multiple tests inherit the environment variable and bind to the same address
There are three groups of tests that use the real server :
LambdaRuntimeTests.swift
LambdaRuntime+ServiceLifecycle.swift
LambdaLocalServerTests.swift
(this PR)
I will add a commit to group them in the same .serialized
suite.
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.
@adam-fowler wdyt ? 2216a5c
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.
Looks good
fixes #556 |
Allows users to define on which port the Local server listens to, using the
LOCAL_LAMBDA_PORT
environment variable.While being at it, I also added
LOCAL_LAMBDA_HOST
if the user wants to bind on a specific IP address.I renamed
LOCAL_LAMBDA_SERVER_INVOCATION_ENDPOINT
toLOCAL_LAMBDA_INVOCATION_ENDPOINT
for consistency.Motivation:
Addresses #556
Modifications:
When run outside of the Lambda execution environment, check for the value of
LOCAL_LAMBDA_PORT
and passes it down to the Lambda HTTP Local Server and runtime client.Add a unit test
Result: