-
Notifications
You must be signed in to change notification settings - Fork 45
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 socket and stream tests #6516
Comments
Maybe this would be a good candidate for a parametrized fixture? something like Originally posted by @surovic in #6511 (comment) |
@surovic I've been thinking about how to refactor this for a while now. I'm unable to come up with something I really like. For the stdin program trace you need both the Regarding the socket read/write tests, my understanding is that we should ideally reach two separate test-cases. When looking at it, I can see us creating two separate fixtures ( Once the fixture-functions have executed and we have the respective program trace we would do exactly the same evaluation for both test cases. That should probably go into a separate function, reducing the test functions to just invoking the evaluation-function and having different fixtures as arguments. From my point of view, all of the above solutions I've been able to come up with adds unnecessary complexity. It might be more 'pure' from a pytest perspective, but I don't think it would make the test cases more comprehendible. Please, if you see any good solutions - do feel free to suggest them. I might be approaching the problem from a bad angle. I'm all for improving readability and ease of comprehending the code we produce. Although, for this particular case I've not been able to come up with anything good. If you (or anyone else) don't have any concrete ideas I suggest we close this issue. |
This seems like it should be two tests (server, client) with a local fixture to set up the common code. I would use parametrize for tests that share ~90% of the code.
Originally posted by @surovic in #6511 (comment)
The text was updated successfully, but these errors were encountered: