Skip to content
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

Move websocket test server to its own package #166

Merged
merged 7 commits into from Dec 23, 2021

Conversation

inancgumus
Copy link
Member

This PR is experimentation that I created for getting feedback.

I plan to add the BrowserType.Connect feature (#17) and put the related tests in the tests pkg.

The first test will create a BrowserType and then connect it to a WebSocket test server. To do that, I need a test WebSocket server, and thanks to Robin, there is actually one here, in the common pkg.

There are also two tests in the common/ pkg that use the server. Since I will use the server from the tests pkg, there will be circular import errors.

So I moved the server into tests/ws. This solves the issues I explained above. I want to hear your input about this before continuing.

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor suggestions.

tests/ws/server.go Outdated Show resolved Hide resolved
tests/ws/server.go Outdated Show resolved Hide resolved
tests/ws/server.go Outdated Show resolved Hide resolved
tests/ws/server.go Outdated Show resolved Hide resolved
inancgumus added a commit that referenced this pull request Dec 23, 2021
inancgumus added a commit that referenced this pull request Dec 23, 2021
tests/ws/server.go Outdated Show resolved Hide resolved
tests/ws/server.go Show resolved Hide resolved
common/session_test.go Show resolved Hide resolved
Co-authored-by: Ivan Mirić <ivan.miric@grafana.com>
@inancgumus inancgumus merged commit 85893f4 into main Dec 23, 2021
@inancgumus inancgumus deleted the refactor/move-ws-test-server branch December 23, 2021 14:26
@inancgumus
Copy link
Member Author

Thank you, @imiric, for reviewing this and helping me see some edge cases I missed.

@inancgumus inancgumus self-assigned this Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants