-
Notifications
You must be signed in to change notification settings - Fork 115
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
Code reuse #834
base: master
Are you sure you want to change the base?
Code reuse #834
Conversation
It seems that its not so easy in all scripts/tests - but maybe it is still worth the effort where it is possible? |
Notice I did search->replace, in some cases the code order was different - and I didn't replace it with the code reuse as I wasn't sure if the order was critical I also ran test prior and after and I haven't noticed any changes/issues |
So, the reason why I rarely used functions to generate common parts of the conversation is for readability: I expect that when a developer of a TLS library is investigating a test failure, they will likely see just one or two failed tests in a script, so they'll want to focus on understanding those specific conversations. I think it will be easier for them if they don't have to understand and inspect multiple places to understand what a particular conversation actually does (part of the problem is that I have no good way to visualise a created decision graph—the code that makes it is the best thing we have). That being said, for some tests, where there really are very minute or subtle changes between the individual conversations (the test-bleichenbacher-* ones are a good example), it may indeed be better to place the common code in a separate function and have just the differences between them highlighted. Are the tests you modified similar to the Bleichenbacher tests, or more varied? (also, sorry for replying just now, but I've been on holidays last week, so I'm now slowly catching up) |
If you look at the changes, and some more improvement can be made to them (i.e improve the code resuse), it really highlights the changes especially when the changes between conversations are "minimal" (i.e. related to Client Key Generator) Some other code reuse can be further done ... so if you are ok with the direction let me know My goal is to make it easier for everyone who code review quickly see the differences between tests/conversations |
Ok, then please split up this PR so that it doesn't modify 16 files at once, and please change the commit message so that it mentions the name of the script updated. I'll be rather busy the next two weeks, so I won't have the time to review a PR this big, but I should be able to find the time to review a PR updating a single script once a day. |
Hi,
May I propose that if its possible - to use a function rather than repeated code to initiating the 'connection'
It seems that a lot of code is re-used inside the test - make it less clear where the 'diff' occurs - making it harder to track when changes occur/bugs are present
What do you think?
This change is