-
Notifications
You must be signed in to change notification settings - Fork 6
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
Better assertion with less wait time #10
Conversation
@@ -4,7 +4,8 @@ | |||
require 'io/console' | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to require 'io/wait'
required for RUBY_VERSION <3.2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍 added
22a1a5a
to
a2ec4bf
Compare
|
||
private def sync(wait = @wait) | ||
sync_until = Time.now + @timeout | ||
while @pty_output.wait_readable(wait) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private def vterm_write(chunk) | ||
@vterm.write(chunk) | ||
@pty_input.write(@vterm.read) | ||
@result = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is exception handling no longer necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vterm.write(chunk)
It always succeeds because it is not an IO, it won't be closed.
Errno::EAGAIN, Errno::EWOULDBLOCK, IO::EAGAINWaitReadable
Not needed because there is always wait_readable before read_nonblock
Errno::EIO
def write(str)
Failed write (always called from test) should always raise an error and make the test fail.
def test_foo
write("exit!\n") # Process terminates. pty_input will be closed.
write("1+2\n") # This should raise error
end
@pty_input.write(@vterm.read)
There is a chance that error might raise. I added rescue Errno::EIO
.
(It rarely happens. I can't reproduce raising Errno::EIO without inserting sleep 0.0004
before calling vterm_write)
lib/yamatanooroti/vterm.rb
Outdated
assert_screen_with_proc( | ||
->(a) { expected_lines == a }, | ||
->(a) { assert_equal(expected_lines, a, message) } | ||
) | ||
when String | ||
assert_equal(expected_lines, actual_lines.join("\n").sub(/\n*\z/, "\n"), message) | ||
assert_screen_with_proc( | ||
->(a) { expected_lines == a }, | ||
->(a) { assert_equal(expected_lines, a, message) }, | ||
lines_to_string | ||
) | ||
when Regexp | ||
assert_screen_with_proc( | ||
->(a) { expected_lines.match?(a) }, | ||
->(a) { assert_match(expected_lines, a, message) }, | ||
lines_to_string | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does a
mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actual_lines
or convert_proc.call(actual_lines)
. I changed it to simply actual
lib/yamatanooroti/vterm.rb
Outdated
result | ||
end | ||
|
||
private def assert_screen_with_proc(check_proc, assert_block, convert_proc = :itself.to_proc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like using assert
gives the impression of a public method for testing, so maybe a different name would be better. (Sorry, I don’t have any ideas right now.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to retryable_screen_assertion_with_proc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! Thank you!
Need to merge ruby/irb#1000 and ruby/reline#746 first
There are good E2E tests and bad E2E tests.
Yamatanooroti should provide a way to write good E2E test for fast and stable testing.
write
should not sleep 0.1sec(before write) + 0.1sec(after write). It should be faster.assert_screen
should retry with a very short wait time.assert_screen
should accept regexp to avoidsleep t; assert_match(result.join, /pattern/)
assert_screen
andresult
several times in a single test case.Example using the new feature
IRB and Reline test
IRB and Reline's test implicitly depend on
wait=0.1
, so test will fail. We need to properly set startup_message and/or add an assert_screen before some write operations.Expected performance improvements are:
Reline test_yamatanooroti: 3:30 -> 0:50
IRB test_yamatanooroti: 0:38 -> 0:12
Branches:
https://github.com/tompng/reline/tree/new_yamatanooroti
https://github.com/tompng/irb/tree/new_yamatanooroti