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

Test with JRuby #22

Merged
merged 18 commits into from Feb 5, 2021
Merged

Test with JRuby #22

merged 18 commits into from Feb 5, 2021

Conversation

nobu
Copy link
Member

@nobu nobu commented Jan 19, 2021

Enabled CI with JRuby, but seems hanging up.
@headius, do you have any idea?

@nobu nobu marked this pull request as draft January 19, 2021 01:39
@nobu nobu requested a review from headius January 19, 2021 01:39
@headius
Copy link
Contributor

headius commented Jan 26, 2021

@nobu I will look into this. Thank you for starting the process!

@headius
Copy link
Contributor

headius commented Jan 26, 2021

Two tests appear to hang: test_raw_minchar and test_raw_timeout

Both of these tests use a complicated set of queues and thread status checks that very likely may have races. I am trying to sort out what these tests are actually doing to see if there is a better way to accomplish it.

Skipping those tests lets the suite complete (with a handful of errors) on SEED=1 at least.

@headius
Copy link
Contributor

headius commented Jan 26, 2021

It appears that test_raw_minchar is hanging because the getch impl does not handle the min: option.

@headius
Copy link
Contributor

headius commented Jan 26, 2021

test_raw_timeout also hangs at the getch call.

@headius
Copy link
Contributor

headius commented Jan 26, 2021

This stupid patch makes the tests run to completion:

diff --git a/lib/ruby/stdlib/io/console/jruby/common.rb b/lib/ruby/stdlib/io/console/jruby/common.rb
index 10f7fc0396..48eeedbef4 100644
--- a/lib/ruby/stdlib/io/console/jruby/common.rb
+++ b/lib/ruby/stdlib/io/console/jruby/common.rb
@@ -1,6 +1,7 @@
 # Methods common to all backend impls
 class IO
-  def getch(*)
+  def getch(*, min: 1)
+    return nil if min == 0
     raw do
       getc
     end

I will work on a less hacky fix.

@headius
Copy link
Contributor

headius commented Jan 27, 2021

I added support for min: which should allow the test suite to complete. I am looking into the other failures now.

@headius
Copy link
Contributor

headius commented Jan 27, 2021

The tests test_cursor_position and test_getpass both fail with CRuby on MacOS, assuming I am running the tests right locally. Appears to raise errors in the subprocess but I have not dug deeper than that.

@headius
Copy link
Contributor

headius commented Jan 27, 2021

I do not understand why my JRuby-specific changes broke the other platforms.

@headius
Copy link
Contributor

headius commented Jan 27, 2021

Three out of the four JRuby runs (push/pull_request on head/9.2) now run to completion (with known errors) but I still don't understand why my changes broke the other builds. No other runs should even be loading the JRuby sources.

@headius
Copy link
Contributor

headius commented Jan 27, 2021

Ok at least some of the problems are due to the subprocesses launched by the tests not including the jruby dir, so they pick up the older version shipped with JRuby instead of the one in the repository.

I am testing a change that moves the JRuby stuff back under lib like it was in my original PR to see if it improves things.

@headius
Copy link
Contributor

headius commented Jan 27, 2021

On JRuby the test test_stringio_getch is failing because there are warnings: power_assert needs tracing which needs --debug on JRuby, and test/unit is triggering an old variable shadowing warning:

power_assert-1.1.3/lib/power_assert.rb:8: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
power_assert-1.1.3/lib/power_assert.rb:8: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
test-unit-3.2.9/lib/test/unit/data-sets.rb:86: warning: shadowing outer local variable - variables

Ignoring stderr makes this pass on JRuby. I do not know why it started failing on CRuby.

nobu and others added 5 commits January 29, 2021 13:00
This allows the test suite to complete rather than blocking on the
getch calls.
power_assert enables traces, which are only partially supported on
JRuby without --debug. The warning that results breaks
assert_separately due to stderr output from the child process.
This works at command line but seems to block in the cursor
positioning tests.
@headius
Copy link
Contributor

headius commented Feb 3, 2021

And weirdly the CRuby tests now run to completion again.

I am filling out some missing functionality to get closer to passing all tests.

Having the separate dir makes testing difficult and doesn't
reflect the structure the gem will eventually have. We can filter
these files out if necessary when building the CRuby gem.
@headius
Copy link
Contributor

headius commented Feb 3, 2021

Locally I am down to 3 failures, but the most recent change that allows it to load the right console logic on JRuby causes the cursor position test to hang. For some reason even though it appears to write the proper escape sequence, that can't be read from the parent and so it gets stuck on readpartial.

The subprocess script here works fine at a command line, but when
run as a pty subprocess during the tests the master side hangs
waiting for output.
@nobu
Copy link
Member Author

nobu commented Feb 4, 2021

IO.console seems nil in run_pty.
Pty isn’t working properly?

@headius
Copy link
Contributor

headius commented Feb 4, 2021

Pty isn’t working properly?

I am trying to figure that out now. It passes all test_pty and it appears to do the same as pty.c so I am not sure where the bug is, if any.

@headius
Copy link
Contributor

headius commented Feb 4, 2021

@headius
Copy link
Contributor

headius commented Feb 4, 2021

This script reproduces the hanging in readpartial... on JRuby, both the 'hello' and the cursor escape sequence do not seem to make it into readpartial and it hangs there. Either line works on CRuby.

If the raw call is removed it works fine. I am looking into whether we handle raw mode incorrectly now.

require 'pty'
require 'io/console'

r, w, pid = PTY.spawn("jruby", "-Ilib", "-rio/console", "-e", <<src)
f = File.open('/dev/tty', 'r+')
f.sync = true
f.raw {
  f.syswrite 'hello'
  # f.cursor
}
f.cursor_down(3); con.puts
f.cursor_right(4); con.puts
f.cursor_left(2); con.puts
f.cursor_up(1); con.puts
src

p r.readpartial(5)

The FFI version of io/console now passes rake test on CRuby.
@headius
Copy link
Contributor

headius commented Feb 4, 2021

I have made a few additional fixes and now the FFI version passes all tests under CRuby.

@nobu At this point I think it is probably safe to say the FFI logic is good enough to ship, test issues on JRuby notwithstanding. The remaining problems do seem to be an issue with JRuby's PTY module so I will be looking into that next. I don't think there's any more work to do on the FFI io-console since it seems to work well in CRuby now.

@headius
Copy link
Contributor

headius commented Feb 4, 2021

I have confirmed that CRuby also hangs in the same tests at readpartial when using JRuby's FFI-based PTY.

@headius
Copy link
Contributor

headius commented Feb 4, 2021

Most of our current pty.rb came from @Freaky in jruby/jruby#3185 but due to our inability to fork we are unable to properly set up the tty in the subprocess. Specifically:

  • we can't setsid
  • we can't use ioctl to set the controlling tty

This is a known issue in our PTY and I do not know how to fix it. I have opened an issue for it here: jruby/jruby#6552

This test runs with test/unit now, which defines omit instead of
skip.
JRuby's PTY.spawn does not produce a process with its own
controlling terminal, which is necessary for testing these raw
escape sequences. This commit marks those tests as pending.

The functionality tested appears to work at a command line, but
due to this PTY bug in JRuby we cannot test it this way.

See jruby/jruby#6552
@headius
Copy link
Contributor

headius commented Feb 4, 2021

I have pushed a final set of commits that should make this green everywhere:

  • Replaced skip calls with omit since test/unit does not define skip.
  • Marked the tests that use run_pty as pend on JRuby

The suite runs green with me locally now and should be green in CI. I believe this PR is done.

@headius headius marked this pull request as ready for review February 4, 2021 21:08
@headius headius mentioned this pull request Feb 4, 2021
@nobu nobu merged commit b5c8e7b into master Feb 5, 2021
@nobu nobu deleted the test-with-jruby branch February 5, 2021 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants