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

Code reuse #834

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Code reuse #834

wants to merge 23 commits into from

Conversation

nrathaus
Copy link
Contributor

@nrathaus nrathaus commented Aug 7, 2023

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 Reviewable

@nrathaus
Copy link
Contributor Author

nrathaus commented Aug 7, 2023

It seems that its not so easy in all scripts/tests - but maybe it is still worth the effort where it is possible?

@nrathaus
Copy link
Contributor Author

nrathaus commented Aug 9, 2023

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

@tomato42
Copy link
Member

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)

@nrathaus
Copy link
Contributor Author

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

@tomato42
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants