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

Fix Test timedout in test_debug_cmd #582

Merged
merged 2 commits into from May 17, 2023

Conversation

tompng
Copy link
Member

@tompng tompng commented May 16, 2023

Description

In ruby head, readline-ext is removed. Readline.readline will be Reline.readline instead of readline.so.

The problem is that Reline prints "▽\e[6n", east-asian-amibuous character and device status report escape sequence.
Reline will wait for cursor position report \e[<row>;<col>R forever and cause timeout error.
Timeout result in #581
To suppress this, I override Reline::IOGate.cursor_pos to use Reline::GeneralIO.cursor_pos in test_debug_cmd

Failing test (help wanted)

Still one test is failing in ruby head.

Failed assertion

assert_match(/\(rdbg\) next/, output)

Actual output

From: /tmp/irb-20230516-1760-qqd7e7.rb @ line 1 :

 => 1: binding.irb
    2: puts "hello"

debug
next
continue
(rdbg) [1, 2] in /tmp/irb-20230516-1760-qqd7e7.rb
     1| binding.irb
=>   2| puts "hello"
=>#0	<main> at /tmp/irb-20230516-1760-qqd7e7.rb:2
(rdbg) hello

This is because ruby/debug is directly requiring readline.so and fallbacks to print prompt; gets.
https://github.com/ruby/debug/blob/4ec9d7ab46df09aa87d64e600df8805a3ee0314c/lib/debug/console.rb#L164-L168

@st0012
Copy link
Member

st0012 commented May 16, 2023

@tompng I think if you also apply this change it'll pass:

diff --git a/test/irb/test_debug_cmd.rb b/test/irb/test_debug_cmd.rb
index 309e241..1cf6066 100644
--- a/test/irb/test_debug_cmd.rb
+++ b/test/irb/test_debug_cmd.rb
@@ -17,7 +17,7 @@ module TestIRB
 
   class DebugCommandTestCase < TestCase
     IRB_AND_DEBUGGER_OPTIONS = {
-      "RUBY_DEBUG_NO_RELINE" => "true", "NO_COLOR" => "true", "RUBY_DEBUG_HISTORY_FILE" => ''
+      "NO_COLOR" => "true", "RUBY_DEBUG_HISTORY_FILE" => ''
     }

A few questions:

  1. What does this do?

    Reline prints "▽\e[6n", east-asian-amibuous character and device status report escape sequence.
    Reline will wait for cursor position report \e[;R forever and cause timeout error.

  2. Should Reline.readline behave this way?

  3. I found forcing Reline::IOGate to be GeneralIO also prevents the issue:

    @@ -214,11 +214,7 @@ module TestIRB
           rc_file.write(<<~RUBY)
             IRB.conf[:USE_SINGLELINE] = true
             require 'readline'
    -        if Readline.const_defined?(:IOGate)
    -          def (Reline::IOGate).cursor_pos
    -            Reline::GeneralIO.cursor_pos
    -          end
    -        end
    +        Reline.const_set(:IOGate, Reline::GeneralIO)
           RUBY
           rc_file.close

    If the answer of 2) is yes, perhaps we can make Reline's IOGate configurable so it works in testing environment too? ruby/debug could benefit from this.

@tompng
Copy link
Member Author

tompng commented May 16, 2023

What does this do?

It prints east asian ambiguous character, request current cursor position by printing "\e[6n", get the actual cursor position escape sequence from STDIN, and determine the actual east asian ambiguous character width.
User can set east asian ambiguous width to either half-width or full-width in preference panel of terminal emulator app. Reline should know this setting value.

Should Reline.readline behave this way?

I think yes. By doing it, reline can handle east asian ambiguous chars correctly.
On the other hand, readline does not do this. It assumes ambiguous char is half-width and it fails to render in full-width setting.
Image of readline fails handling full-width

If STDIN and STDOUT is a TTY, reline is assuming it is run inside a terminal that answers to "\e[6n". I think this assumption is correct and what is wrong is that the test is using pty but does not act like a real terminal that reply to "\e[6n".

  1. Reline::IOGate to be GeneralIO

Yes, it is one of the solution. Making configurable seems good.
We can also force reline to use GeneralIO by not using PTY.spawn but use IO.popen.
Unfortunately, I tried IO.popen but there was a problem of buffering.

IO.popen ['ruby', '-e', '5.times{sleep 1;puts _1}'], 'w+' do |io|
  5.times { puts io.gets }
  # expect: prints one line per sec for 5 times
  # actual: wait for 5 sec and prints 5 lines at once
  # It means, `line.match?(/binding\.irb/)` in test_debug_cmd will timeout.
end

@tompng
Copy link
Member Author

tompng commented May 16, 2023

Thanks for the diff, test passed!

@st0012
Copy link
Member

st0012 commented May 17, 2023

Thanks for the fix and explanation 🙏
Yeah I think ideally we should:

  • Either more realisticly simulate users’ activity in tests, which in this case means doing cursor position report (does this make sense though?)
  • Or we should switch to popen

But we can do that in a follow up PR and let’s fix the build first 👍

@st0012 st0012 merged commit d3eed13 into ruby:master May 17, 2023
24 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request May 17, 2023
(ruby/irb#582)

* Suppress Reline::IOGate.cursor_pos writing escape sequence in test_debug_cmd

* Force use Reline::GeneralIO as Reline::IOGate and remove RUBY_DEBUG_NO_RELINE option for debug test
nekoyama32767 added a commit to nekoyama32767/ruby_uniform that referenced this pull request May 19, 2023
author nekoyama32767 <nekoyama32767@gmail.com> 1683953864 +0900
committer nekoyama32767 <nekoyama32767@gmail.com> 1684490735 +0900

parent 6a58ac0
author nekoyama32767 <nekoyama32767@gmail.com> 1683953864 +0900
committer nekoyama32767 <nekoyama32767@gmail.com> 1684490702 +0900

parent 6a58ac0
author nekoyama32767 <nekoyama32767@gmail.com> 1683953864 +0900
committer nekoyama32767 <nekoyama32767@gmail.com> 1684489980 +0900

Direct primitive compare sort

uniform check

uniform sort fix

move intro-sort to enum,c

apply commit suggestion

sparate member in sort_by_data

expand OPTIMIZED_CMP

optimize rb_uniform_num_cmp_bool

optimize rb_uniform_num_cmp_bool

optimize rb_uniform_num_cmp_bool

optimize rb_uniform_num_cmp_bool

Add init optimiz check

remove compare optimiz check

bit operation check uniform

adjust code style

adjust code style

adjust code style

adjust code style

adjust code style

adjust code style

adjust code style

adjust code style

adjust code style

code style

code style

remove rb_funcallv from rb_uniform_is_less

apply rb_uniform_sort_data as wrapper

fix ptr

uniform sort wrapper

use rb_uniform_is_larger to check sort needed

Implements [Feature #19643] indexing ptr

Implements [Feature #19643] Add Benchmark

Introduce math_arc macro

Update bundled gems list at 2023-05-13

optimize rb_uniform_num_cmp_bool

Add init optimiz check

bit operation check uniform

adjust code style

adjust code style

adjust code style

adjust code style

adjust code style

Introduce anddot_multiple_assignment_check function

[wasm] Fix `unreachable` error during printing setjmp trace message

Preprocess input parse.y from stdin

Add user argument to some macros used by bison

[Bug #19025] Numbered parameter names are always local variables

`rb_io_puts` should not write zero length strings. (ruby#7806)

Bump ruby/setup-ruby from 1.148.0 to 1.149.0

Bumps [ruby/setup-ruby](https://github.com/ruby/setup-ruby) from 1.148.0 to 1.149.0.
- [Release notes](https://github.com/ruby/setup-ruby/releases)
- [Commits](ruby/setup-ruby@d2b39ad...7d546f4)

---
updated-dependencies:
- dependency-name: ruby/setup-ruby
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Unskip the test skipped in ruby#4173 (ruby#7809)

This test was skipped 12 years ago because it was flaky on FreeBSD and
OpenBSD. Since then, Ruby's SIGCHLD handling has been substantially
re-written (mostly by Eric Wong @normalperson in 44fc3d0).

These tests now in fact pass reliably on Ruby master on FreeBSD 13.2 and
OpenBSD 7.3. I stress-tested the test_wait_and_sigchild test on my
laptop by running four copies of the test in a loop on a 8-core VM; both
by itself and also as part of the whole test_process.rb file. I did not
see any failures.

Let's unskip the test and close [ruby#4173] out. I'll keep an eye out on Ruby
CI for any flakes in this file on BSD after this gets merged, but if we
don't see any I'm going to assume 44fc3d0 or related changes around
that time accidently fixed this bug.

It's also probably important to unskip this test so that if another
attempt at removing the special SIGCHLD handling is made (like was
reverted in ruby#7517), we get signal if
that breaks on FreeBSD/OpenBSD.

[Fixes ruby#4173]

Use the rb_sys_fail_str macro in signal.c

Let signal.c include "internal/error.h" explicitly to ensure that the
identifier rb_sys_fail_str in signal.c refers to the macro defined in
"internal/error.h" instead of the actual function.

That macro reads errno before evaluating its argument.  Without this
change, the rb_signo2signm(sig) expression in the "trap" function in
signal.c will overwrite the errno before the actual rb_sys_fail_str
function reads the errno.

Lrama v0.5.0 (ruby#7814)

Process parse.y without temporary files

[DOC] Fix a link [ci skip]

Remove explicit SIGCHLD handling. (ruby#7816)

* Remove unused SIGCHLD handling.

* Remove unused `init_sigchld`.

* Remove unnecessary `#define RUBY_SIGCHLD (0)`.

* Remove unused `SIGCHLD_LOSSY`.

[wasm] Allocate asyncify buffer on heap to save stack usage

[rubygems/rubygems] Bump rb-sys

Bumps [rb-sys](https://github.com/oxidize-rb/rb-sys) from 0.9.75 to 0.9.77.
- [Release notes](https://github.com/oxidize-rb/rb-sys/releases)
- [Commits](oxidize-rb/rb-sys@v0.9.75...v0.9.77)

---
updated-dependencies:
- dependency-name: rb-sys
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Ensure SIGCHLD always uses a signal handler. (ruby#7819)

Implements [Feature #19643] Add Benchmark

[ruby/irb] Fix Test timedout in test_debug_cmd
(ruby/irb#582)

* Suppress Reline::IOGate.cursor_pos writing escape sequence in test_debug_cmd

* Force use Reline::GeneralIO as Reline::IOGate and remove RUBY_DEBUG_NO_RELINE option for debug test

Implement Hash AR tables on VWA

Implement Hash ST tables on VWA

Move ar_hint to ar_table_struct

This allows Hashes with ST tables to fit int he 80 byte size pool.

[ruby/irb] Print deprecation warning for `help` command
(ruby/irb#567)

* Give show_doc its own command class

* Print deprecation warning for `help` command

Add Fiber#kill, similar to Thread#kill. (ruby#7823)

Introduce gc_mark_table macro

change to test/objectspace, don't rely on Object's shape not being "too complex"

[ruby/irb] Refactor RubyLex's input/io methods
(ruby/irb#583)

1. Make `RubyLex#set_input` simply assign the input block. This matches
   the behavior of `RubyLex#set_prompt`.
2. Merge `RubyLex#set_input`'s IO configuration logic with `#set_auto_indent`
   into `#configure_io`.

YJIT: Enable debug symbols in dev_nodebug (ruby#7822)

[ruby/openssl] CI: Add OpenSSL FIPS mode case.

test/openssl/fixtures/ssl/openssl_fips.cnf.tmpl:

I referred to the following document for the openssl config file for FIPS mode.
<https://www.openssl.org/docs/manmaster/man7/fips_module.html>
- Making all applications use the FIPS module by default

It seems that the `.include` syntax only requires the absolute path.
So, the placeholder OPENSSL_DIR in the template file is replaced with the
actual OpenSSL directory.

.github/workflows/test.yml:

The `TEST_RUBY_OPENSSL_FIPS_ENABLED` environment variable is set
in the FIPS mode CI case. It can be used in the unit tests.

ruby/openssl@18b017218c

[ruby/openssl] Implement FIPS functions on OpenSSL 3.

This commit is to implement the `OpenSSL::OPENSSL_FIPS`, `ossl_fips_mode_get`
and `ossl_fips_mode_set` to pass the test `test/openssl/test_fips.rb`.

It seems that the `OPENSSL_FIPS` macro is not used on the FIPS mode case any
more, and some FIPS related APIs also were removed in OpenSSL 3.

See the document <https://github.com/openssl/openssl/blob/master/doc/man7/migration_guide.pod#removed-fips_mode-and-fips_mode_set>
the section OPENSSL 3.0 > Main Changes from OpenSSL 1.1.1 >
Other notable deprecations and changes - Removed FIPS_mode() and FIPS_mode_set() .

The `OpenSSL::OPENSSL_FIPS` returns always true in OpenSSL 3 because the used
functions `EVP_default_properties_enable_fips` and `EVP_default_properties_is_fips_enabled`
works with the OpenSSL installed without FIPS option.

The `TEST_RUBY_OPENSSL_FIPS_ENABLED` is set on the FIPS mode case on the CI.
Because I want to test that the `OpenSSL.fips_mode` returns the `true` or
'false' surely in the CI. You can test the FIPS mode case by setting
`TEST_RUBY_OPENSSL_FIPS_ENABLED` on local too. Right now I don't find a better
way to get the status of the FIPS mode enabled or disabled for this purpose. I
am afraid of the possibility that the FIPS test case is unintentionally skipped.

I also replaced the ambiguous "returns" with "should return" in the tests.

ruby/openssl@c5b2bc1268

[ruby/openssl] Fix warnings about the OPENSSL_FIPS macro in OpenSSL 1.1.

The commit <ruby/openssl@c5b2bc1268bc> made the warnings below
in the case of OpenSSL 1.1 where the `OPENSSL_FIPS` macro is not defined.

```
$ bundle install --standalone

$ bundle exec rake compile -- \
  --with-openssl-dir=$HOME/.local/openssl-1.1.1t-debug \
  --with-cflags="-Wundef"
mkdir -p tmp/x86_64-linux/openssl/3.2.1
cd tmp/x86_64-linux/openssl/3.2.1
/usr/local/ruby-3.2.1/bin/ruby -I. -r.rake-compiler-siteconf.rb ../../../../ext/openssl/extconf.rb -- --with-openssl-dir=/home/jaruga/.local/openssl-1.1.1t-debug --with-cflags=-Wundef
...
gcc -I. -I/usr/local/ruby-3.2.1/include/ruby-3.2.0/x86_64-linux -I/usr/local/ruby-3.2.1/include/ruby-3.2.0/ruby/backward -I/usr/local/ruby-3.2.1/include/ruby-3.2.0 -I../../../../ext/openssl -DRUBY_EXTCONF_H=\"extconf.h\" -I/home/jaruga/.local/openssl-1.1.1t-debug/include    -fPIC -Wundef  -o ossl.o -c ../../../../ext/openssl/ossl.c
../../../../ext/openssl/ossl.c: In function ‘ossl_fips_mode_get’:
../../../../ext/openssl/ossl.c:425:7: warning: "OPENSSL_FIPS" is not defined, evaluates to 0 [-Wundef]
  425 | #elif OPENSSL_FIPS
      |       ^~~~~~~~~~~~
../../../../ext/openssl/ossl.c: In function ‘ossl_fips_mode_set’:
../../../../ext/openssl/ossl.c:460:7: warning: "OPENSSL_FIPS" is not defined, evaluates to 0 [-Wundef]
  460 | #elif OPENSSL_FIPS
      |       ^~~~~~~~~~~~
../../../../ext/openssl/ossl.c: In function ‘Init_openssl’:
../../../../ext/openssl/ossl.c:1218:7: warning: "OPENSSL_FIPS" is not defined, evaluates to 0 [-Wundef]
 1218 | #elif OPENSSL_FIPS
      |       ^~~~~~~~~~~~
...
cp tmp/x86_64-linux/openssl/3.2.1/openssl.so tmp/x86_64-linux/stage/lib/openssl.so
```

ruby/openssl@b4228cbcd6

[ruby/openssl] Revert "Skip OpenSSL::TestHMAC#test_dup when running with RHEL9"

This reverts commit ruby/openssl@9493d4a3bb26.

ruby/openssl@b880a023dd

[ruby/irb] Display mod key as `Option` on Darwin platforms
(ruby/irb#584)

Check RUBY_PLATFORM for `darwin` and modify the mod key from `Alt` to
`Option`.

Skip test_dump_too_complex_shape for YJIT for now

It fails too often with YJIT:

* https://github.com/ruby/ruby/actions/runs/5015976941/jobs/8992254690
* https://github.com/ruby/ruby/actions/runs/5017310353/jobs/8995281395
* https://github.com/ruby/ruby/actions/runs/5019625711/jobs/9000322487
* https://github.com/ruby/ruby/actions/runs/5019883965/jobs/9000836915

ref: ruby#7646
st0012 added a commit to Shopify/rails that referenced this pull request Jun 1, 2023
1. By removing the `--singleline` flag, IRB will use Reline by default.
2. By assigning `TERM=dumb`, Reline will skip east-asian width detection,
   which was what caused the test to hang.

I need to stress that the east-asian width detection is not a bug but an
improvement in Reline to help rendering east-asian characters correctly.
Readline actually can't do this well. Please see @tompng's great explanation
in ruby/irb#582 (comment)

However, this detection should not happen when the terminal is running in
PTY (usually used in test environment). The problem is that in Ruby we
don't have a way to detect if the terminal is running in TTY or PTY.

But by passing `TERM=dumb`, Reline will assume that the terminal is not
capable of several advanced features, including this east-asian width
detection.
@tompng tompng deleted the debug_test_suppress_cursor_pos branch June 1, 2023 20:36
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