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

Better assertion with less wait time #10

Merged
merged 4 commits into from
Sep 16, 2024
Merged

Conversation

tompng
Copy link
Member

@tompng tompng commented Sep 5, 2024

Need to merge ruby/irb#1000 and ruby/reline#746 first

There are good E2E tests and bad E2E tests.

require 'capybara'

# Good E2E test
click_link('bar')
expect(page).to have_content('baz') # This will automatically retries

# Bad and slow E2E test
click_link('bar')
sleep 1 # Need to sleep enough time because there is no retry in the assertion below
expect(page.content).to include('baz')

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 avoid sleep t; assert_match(result.join, /pattern/)
  • Should be able to call assert_screen and result several times in a single test case.

Example using the new feature

start_terminal(rows, cols, command, startup_message: /irb\(main\):001>/)
write "sleep 1\n"
# Wait until irb is ready
assert_screen(/irb\(main\):002>/)
write "debug\n"
# Wait until rdbg is ready
assert_screen(/irb:rdbg/)
write "foobar\n"
# Assert result
assert_screen(expected_result)
close

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

@@ -4,7 +4,8 @@
require 'io/console'

Copy link

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍 added


private def sync(wait = @wait)
sync_until = Time.now + @timeout
while @pty_output.wait_readable(wait)
Copy link
Member

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
Copy link
Member

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?

Copy link
Member Author

@tompng tompng Sep 16, 2024

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)

Comment on lines 142 to 157
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
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does a mean?

Copy link
Member Author

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

result
end

private def assert_screen_with_proc(check_proc, assert_block, convert_proc = :itself.to_proc)
Copy link
Member

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.)

Copy link
Member Author

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

Copy link
Member

@ima1zumi ima1zumi left a 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!

@ima1zumi ima1zumi merged commit 3525535 into ruby:master Sep 16, 2024
9 checks passed
@tompng tompng deleted the better_assertion branch September 16, 2024 14:44
@tompng tompng mentioned this pull request Sep 16, 2024
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.

3 participants