-
-
Notifications
You must be signed in to change notification settings - Fork 298
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 tests #55
Refactor tests #55
Conversation
Great start! I like this change so far. Only thing I would change is the name... maybe something like: |
Makes sense. I wasn't sure about the actual intent of the events but I forgot to ask. I will change it and proceed |
2d227ee
to
6c7d973
Compare
fe3e2e0
to
cb087b2
Compare
cb087b2
to
d6b29e3
Compare
Suggestions for further refactoring? I considered refactoring the flows of packet into a dedicated endless Stream but maybe it would be too complex for the scope of this PR so I think it's better to leave them as they are for now. |
Ahh - gotta love a patch with this ratio of added/removed lines :D @chobeat - This is fantastic! I'm inclined to just merge this as is, because it's so much better than what we currently have. I do have one slightly-nitpicky issue that I can take or leave: I think it usually doesn't make sense to expect an Arc (and usually also a Mutex) as a function parameter or as part of a type. These are things that are more easily managed by the calling function (the test in this case) rather than being encapsulated away. It would be easier for the reader to understand what's going on with all the clones and locks if the Arcs and Mutex were all defined in the tests. So for example, you'd have different functions creating the |
It does makes sense and I agree with you but the real question is: is this clarity worth some hundrends lines of code? Also you should consider the loss of clarity from having the same lines of code "polluting" the test logic. On a per test basis what you say is true but if one has to skim all the tests to find something specific or wants to make sense of what is covered and what is not, your change would make it worse. It's a matter of nitpicking as you said but both examples have pros and cons that to me are equal. As I said I'm very new to rust so I assume your version is more idiomatic and we should go for it, but I let you decide. |
I do like the very concise form the tests are right now with this, and assume you have more context as to the amount of code my suggestion would add. :) Maybe we'll look at it sometime in the future, but for now I really want to go forward with this. Thanks a lot for your contribution! This will certainly make adding/changing tests much easier. |
Currently only a small part of the tests has been refactored. I would like to get feedbacks on the general structure and on how idiomatic it is to create test resources in this way. If it's positive, I will expand the PR by moving more and more resources to the utils file.