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

Properly restore Reline::IOGate in test teardown #593

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

tompng
Copy link
Member

@tompng tompng commented Sep 26, 2023

make test-all TESTS="reline/test_reline.rb irb/test_cmd.rb" was failing. Pager is shown and test will stop until you enter q.

The reason of the failure is:

  • Reline sets IOGate to GeneralIO in teardown
  • GeneralIO.get_screen_size returns [1, 1]
  • If show-source content exceeds screen height, IRB uses pager

This pull request fixes test teardown to restore IOGate correctly.

show-source's test is also fixed in ruby/irb#720

@ima1zumi
Copy link
Member

When I run make test-all -j8 with this change in ruby/ruby, reline test always seems to fail. master(5b36c11e21) does not fail.
Do you know what could be the cause?

❯ make test-all -j8
        BASERUBY = /Users/mi/.asdf/shims/ruby --disable=gems
        CC = gcc-13
        LD = ld
        LDSHARED = gcc-13 -dynamiclib
        CFLAGS = -O3 -fno-fast-math -ggdb3 -Wall -Wextra -Wdeprecated-declarations -Wdiv-by-zero -Wduplicated-cond -Wimplicit-function-declaration -Wimplicit-int -Wpointer-arith -Wwrite-strings -Wold-style-definition -Wimplicit-fallthrough=0 -Wmissing-noreturn -Wno-cast-function-type -Wno-constant-logical-operand -Wno-long-long -Wno-missing-field-initializers -Wno-overlength-strings -Wno-packed-bitfield-compat -Wno-parentheses-equality -Wno-self-assign -Wno-tautological-compare -Wno-unused-parameter -Wno-unused-value -Wsuggest-attribute=format -Wsuggest-attribute=noreturn -Wunused-variable -Wmisleading-indentation -Wundef   -fno-common -pipe
        XCFLAGS = -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-strict-overflow -fvisibility=hidden -fexcess-precision=standard -DRUBY_EXPORT -I. -I.ext/include/x86_64-darwin21 -I../ruby/include -I../ruby -I../ruby/yarp -I../ruby/enc/unicode/15.0.0
        CPPFLAGS = -D_XOPEN_SOURCE -D_DARWIN_C_SOURCE -D_DARWIN_UNLIMITED_SELECT -D_REENTRANT
        DLDFLAGS = -Wl,-multiply_defined,suppress -Wl,-undefined,dynamic_lookup -install_name /Users/mi/ghq/github.com/ruby/build/../install/lib/libruby.3.3.dylib -compatibility_version 3.3 -current_version 3.3.0  -fstack-protector-strong -framework CoreFoundation  -fstack-protector-strong -framework CoreFoundation
        SOLIBS = -ldl -lobjc -lpthread
        LANG =
        LC_ALL =
        LC_CTYPE = ja_JP.UTF-8
        MFLAGS = - --jobserver-fds=4,5 -j
        RUSTC = rustc
        YJIT_RUSTC_ARGS = --crate-name=yjit --crate-type=staticlib --edition=2021 -g -C lto=thin -C opt-level=3 -C overflow-checks=on '--out-dir=/Users/mi/ghq/github.com/ruby/build/yjit/target/release/' ../ruby/yjit/src/lib.rs
generating enc.mk
gcc-13 (Homebrew GCC 13.1.0) 13.1.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

making enc
making srcs under enc
make[1]: Nothing to be done for `enc'.
make[1]: Nothing to be done for `srcs'.
generating transdb.h
transdb.h unchanged
making trans
generating makefiles ext/configure-ext.mk
ext/configure-ext.mk updated
make[1]: Nothing to be done for `../ruby/enc/trans'.
making encs
make[1]: Nothing to be done for `encs'.
generating makefile exts.mk
exts.mk unchanged
make[2]: `ruby' is up to date.
make[1]: Nothing to be done for `note'.
Run options:
  --seed=44736
  "--ruby=./miniruby -I../ruby/lib -I. -I.ext/common  ../ruby/tool/runruby.rb --extout=.ext  -- --disable-gems"
  --excludes-dir=../ruby/test/.excludes
  --name=!/memory_leak/

# Running tests:

Leaked file descriptor: TestSocket_BasicSocket#test_close_read: 13 : #<BasicSocket:fd 13>(not-autoclose)
COMMAND   PID USER   FD    TYPE             DEVICE SIZE/OFF NODE NAME
ruby    92174   mi   13u  systm 0x1531a84f82cc5ba9      0t0      [ctl com.apple.netsrc id 6 unit 101]
/Users/mi/ghq/github.com/ruby/ruby/lib/reline.rb:106:in `encode': no implicit conversion of nil into String (TypeError) 92181=test_file_exhaustive 921
        /Users/mi/ghq/github.com/ruby/ruby/lib/reline.rb:106:in `basic_word_break_characters='
        /Users/mi/ghq/github.com/ruby/ruby/lib/reline.rb:560:in `block in core'
        /Users/mi/ghq/github.com/ruby/ruby/lib/reline.rb:80:in `initialize'
        /Users/mi/ghq/github.com/ruby/ruby/lib/reline.rb:555:in `new'
        /Users/mi/ghq/github.com/ruby/ruby/lib/reline.rb:555:in `core'
        /Users/mi/ghq/github.com/ruby/ruby/test/irb/test_history.rb:22:in `<class:TestInputMethodWithRelineHistory>'
        /Users/mi/ghq/github.com/ruby/ruby/test/irb/test_history.rb:20:in `<class:HistoryTest>'
        /Users/mi/ghq/github.com/ruby/ruby/test/irb/test_history.rb:11:in `<module:TestIRB>'
        /Users/mi/ghq/github.com/ruby/ruby/test/irb/test_history.rb:10:in `<top (required)>'
        <internal:/Users/mi/ghq/github.com/ruby/ruby/lib/rubygems/core_ext/kernel_require.rb>:128:in `require'
        <internal:/Users/mi/ghq/github.com/ruby/ruby/lib/rubygems/core_ext/kernel_require.rb>:128:in `require'
        /Users/mi/ghq/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb:122:in `run'
        /Users/mi/ghq/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb:215:in `<main>'
running file: /Users/mi/ghq/github.com/ruby/ruby/test/irb/test_history.rb

A test worker crashed. It might be an interpreter bug or
a bug in test/unit/parallel.rb. Try again without the -j
option.

make: *** [yes-test-all] Error 1

❯ ./miniruby -v
ruby 3.3.0dev (2023-09-27T12:53:01Z master 5b36c11e21) [x86_64-darwin21]
~/ghq/github.com/ruby/ruby master* ⇣
❯ git diff
diff --git a/test/reline/helper.rb b/test/reline/helper.rb
index bb23893187..0a09185f92 100644
--- a/test/reline/helper.rb
+++ b/test/reline/helper.rb
@@ -21,21 +21,23 @@
 module Reline
   class <<self
     def test_mode(ansi: false)
-        remove_const('IOGate') if const_defined?('IOGate')
-        const_set('IOGate', ansi ? Reline::ANSI : Reline::GeneralIO)
-        if ENV['RELINE_TEST_ENCODING']
-          encoding = Encoding.find(ENV['RELINE_TEST_ENCODING'])
-        else
-          encoding = Encoding::UTF_8
-        end
-        Reline::GeneralIO.reset(encoding: encoding) unless ansi
-        core.config.instance_variable_set(:@test_mode, true)
-        core.config.reset
+     @original_iogate = IOGate
+      remove_const('IOGate')
+      const_set('IOGate', ansi ? Reline::ANSI : Reline::GeneralIO)
+      if ENV['RELINE_TEST_ENCODING']
+        encoding = Encoding.find(ENV['RELINE_TEST_ENCODING'])
+      else
+        encoding = Encoding::UTF_8
+      end
+      Reline::GeneralIO.reset(encoding: encoding) unless ansi
+      core.config.instance_variable_set(:@test_mode, true)
+      core.config.reset
     end

     def test_reset
-      remove_const('IOGate') if const_defined?('IOGate')
-      const_set('IOGate', Reline::GeneralIO)
+      remove_const('IOGate')
+      const_set('IOGate', @original_iogate)
+      Reline::GeneralIO.reset
       Reline.instance_variable_set(:@core, nil)
     end

diff --git a/test/reline/test_history.rb b/test/reline/test_history.rb
index ddf8fb1472..390962a633 100644
--- a/test/reline/test_history.rb
+++ b/test/reline/test_history.rb
@@ -3,7 +3,7 @@

 class Reline::History::Test < Reline::TestCase
   def setup
-    Reline.send(:test_mode)
+    Reline.test_mode
   end

   def teardown
diff --git a/test/reline/test_reline_key.rb b/test/reline/test_reline_key.rb
index fb700a6f2e..7f9a11394a 100644
--- a/test/reline/test_reline_key.rb
+++ b/test/reline/test_reline_key.rb
@@ -3,6 +3,7 @@

 class Reline::TestKey < Reline::TestCase
   def setup
+    Reline.test_mode
   end

   def teardown

@tompng
Copy link
Member Author

tompng commented Sep 27, 2023

I think GeneralIO.reset has a bug.
GeneralIO.reset does not reset @@encoding to undefined. Instead, it just sets the value to nil.

def self.reset(encoding: nil)
  @@pasting = false
  @@encoding = encoding # if @@encoding is changed to nil
end

def self.encoding
  if defined?(@@encoding) # @@encoding is still defined
    @@encoding # GeneralIO.encoding returns nil
  ...

It should do remove_class_variable. updated

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.

Thanks!

@ima1zumi ima1zumi merged commit c16d33d into ruby:master Sep 28, 2023
30 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Sep 28, 2023
(ruby/reline#593)

* Properly restore Reline::IOGate in test teardown

* GeneralIO.reset should reset class variable existence

ruby/reline@c16d33dae5
@ima1zumi ima1zumi added the bug Something isn't working label Oct 3, 2023
@tompng tompng deleted the fix_reset_iogate branch March 24, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants