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

Refactor websocket tests to use table testing style #23

Closed
wants to merge 1 commit into from

Conversation

hoanhan101
Copy link

No description provided.

Copy link
Contributor

@edaniszewski edaniszewski left a comment

Choose a reason for hiding this comment

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

I'm not sure it makes sense to merge this. While the changes themselves seem good, I don't think it makes sense to format these tests as a single table test. Mostly because each test case is testing something different (e.g. client.Status, client.Version, ...), so it makes sense for them to be in their own test cases.

It can get kinda messy having everything spread apart in their own case and having duplicate test code for the test execution portion, but it is ultimately more manageable/helpful. Consider if one test here failed -- it would be more difficult to determine where the failure happened since any failure would fail the entire test case. If they were left broken up as individual tests, one failure only fails that case, making it easier to identify.

I think the test table style is good when testing the same thing (e.g. function) with different parameters or conditions. I'm going to close the PR as a wontfix

@edaniszewski edaniszewski added the wontfix This will not be worked on label Mar 12, 2019
@hoanhan101 hoanhan101 deleted the ws-test branch March 19, 2019 04:43
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants