improve test framework in backlog #73
Conversation
ono-max
commented
Jun 8, 2021
•
ono-max
commented
- rename 'lines' to 'backlog'
- use "last_backlog" instead of "internal_info[:backlog]"
- define create_message method to output backlog when assertion failed
test/support/utils.rb
Outdated
| @@ -54,37 +52,41 @@ def create_pseudo_terminal(boot_options: "-r debug/run") | |||
| timeout_sec = (ENV['RUBY_DEBUG_TIMEOUT_SEC'] || 10).to_i | |||
|
|
|||
| PTY.spawn("#{RUBY} #{boot_options} #{temp_file_path}") do |read, write, pid| | |||
| lines = [] | |||
| backlog = [] | |||
There was a problem hiding this comment.
why don't you make @backlog instead of backlog local variable?
There was a problem hiding this comment.
@ko1
In my opinion, local variable is better than instance variable to keep the scope of the variable small.
What do you think about this?
There was a problem hiding this comment.
In this case, @last_backlog and @internal_info are already used and I don't have any good reason why only @backlog is passed as an argument.
There was a problem hiding this comment.
"All in arguments" is another reasonable option for me.
There was a problem hiding this comment.
In this case, @last_backlog and @internal_info are already used and I don't have any good reason why only @backlog is passed as an argument.
@internal_info and @last_backlog are used in some assertions, but @backlog are used in all assertions. Does that make sense for you?
That's why I used @backlog as an argument.
There was a problem hiding this comment.
It doesn't make sense to me. The visibility control is not related to frequency.
backlogis only visible where the value is passed. Well controlled.@internal_infoand@last_backlogare visible from any test class methods.
Visibility of these values should be same. At least I feel it should be same because there is no reason to change the visibility.
There was a problem hiding this comment.
In general, well-controlled visibility is good idea. So passing backlog as an argument is nice.
However, @internal_info and @last_backlog are already exposed, and maybe nobody modify them from test methods (in other words, we can believe test writers). In this background, there is no reason to prohibit exposing backlog and it reduces the code size.
There was a problem hiding this comment.
I fully persuaded myself of your opinion. Thanks!
