-
Notifications
You must be signed in to change notification settings - Fork 35
test: reenable main tests #275
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
main_test was disabled after the global state for the logsapi listener was removed. The new approach is to choose a random port and pass the address to the app. Add a timeout to make sure tests don't hang.
/test |
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 great!
// we cannot return a port that is already in use or it | ||
// would return an error when creating the server. | ||
// The solution is to spawn a test server to get a random | ||
// port and immediately close it so that we can use the 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.
This is racy though, as another process could also be trying to acquire an ephemeral port. If the test ends up being flaky, we may need to have the extension listen on an ephemeral port, log it, and then extract the listening port from the logs. Or something like that.
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.
Agree, the window to hit the race is small enough that I think it's safe to ignore. Another solution I had in mind was to use unix sockets.
main_test was disabled after the global state for the logsapi
listener was removed.
The new approach is to choose a random port and pass the address
to the app.
Add a timeout to make sure tests don't hang.
Blocked by #249
Closes #249