[ruby-core:92676] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[ruby-core:92676] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[merch-redmine]

Issue #15853 has been reported by jeremyevans0 (Jeremy Evans).

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92677] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[mame]

Issue #15853 has been updated by mame (Yusuke Endoh).


```
-        if !defined?(Reline) or Readline != Reline # Reline's rendering logic is tricky
+        unless defined?(Reline) or Readline == Reline or /\A4\.3\z/n.match(Readline::VERSION) # Reline's rendering logic is tricky
```

The new condition looks not to work when `Reline` is not defined.

(Personally I want to avoid `unless` for complex condition :-)

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78039

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92678] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[merch-redmine]

Issue #15853 has been updated by jeremyevans0 (Jeremy Evans).

File fix-readline-4.3-test-regression-v2.patch added

mame (Yusuke Endoh) wrote:
> ```
> -        if !defined?(Reline) or Readline != Reline # Reline's rendering logic is tricky
> +        unless defined?(Reline) or Readline == Reline or /\A4\.3\z/n.match(Readline::VERSION) # Reline's rendering logic is tricky
> ```
> 
> The new condition looks not to work when `Reline` is not defined.
> 
> (Personally I want to avoid `unless` for complex condition :-)

Correct, sorry about that.  How about just eliminating the assertion, since it is readline implementation and version dependent, and of questionable use (who calls `Readline.output.read` after `Readline.readline` and wants the readline prompt and input to appear)?  Attached is a patch for that.

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78040

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92693] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[aycabta]

Issue #15853 has been updated by aycabta (aycabta .).


The condition was for GNU Readline 4.3's bug but the deletion is my mistake. I agreed with your first proposal. But the first patch is not correct. The `defined?(Reline)` doesn't mean that Reline is as Readline so I added a condition that checks `Readline` constant is the same as `Reline`. It's `!defined?(Reline) or Readline != Reline`. So please fix your patch.

The details of the GNU Readline 4.3's bug are as follows.

@naruse mentioned it at a comment in #5785
https://bugs.ruby-lang.org/issues/5785#note-8
and added a commit 2418f9cc5559ff9d9d62f2139ee7d7a971e7be20
https://bugs.ruby-lang.org/projects/ruby-trunk/repository/git/revisions/2418f9cc5559ff9d9d62f2139ee7d7a971e7be20
and showed log.
http://www.rubyist.net/%7Eakr/chkbuild/debian/ruby-trunk/log/20120501T132800Z.diff.html.gz

> ```
> + <n>) Failure:
> +test_modify_text_in_pre_input_hook(TestReadline) [/extdisk/chkbuild/chkbuild/tmp/build/<buildtime>/ruby/test/readline/test_readline.rb:386]:
> +<"> hello world\n"> expected but was
> +<"> ">.
> ```

It's a strange behavior and I think it's a GNU Readline's bug. For your information, rlwrap refer to that:
https://github.com/hanslub42/rlwrap/blob/2aea6fc26c5448c16d482f7ec6b1e16adfee0d17/BUGS#L29-L37

> Christoffer Dam Bruun reported:
> 
>   "I have just installed rlwrap along with readline on Solaris 8 and
>   found that rlwrap did not work properly with readline 4.3. After
>   linking rlwrap with readline 4.2 it works correctly.
> 
>   What happended using readline 4.3 was that an extra prompt would be 
>   written after the first letter on a line, e.g. with a sqlplus prompt:
>   SQL>
>   (now writing select )
> SQL> sSQL> select"

GNU Readline 4.3 has many output bugs.
http://git.savannah.gnu.org/cgit/readline.git/tree/CHANGES?id=bcd7f75a2bc2f7c67c9cd6899ff546afa45cbba4

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78052

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92699] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[merch-redmine]

Issue #15853 has been updated by jeremyevans0 (Jeremy Evans).

File fix-readline-4.3-test-regression-v3.patch added

aycabta (aycabta .) wrote:
> The condition was for GNU Readline 4.3's bug but the deletion is my mistake. I agreed with your first proposal. But the first patch is not correct. The `defined?(Reline)` doesn't mean that Reline is as Readline so I added a condition that checks `Readline` constant is the same as `Reline`. It's `!defined?(Reline) or Readline != Reline`. So please fix your patch.

If you think the assertion is useful, we can definitely keep it and just fix the condition.  I think the attached patch should be correct.

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78057

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)
fix-readline-4.3-test-regression-v3.patch (1.33 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92700] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[mame]

Issue #15853 has been updated by mame (Yusuke Endoh).


@jeremyevans0 I think you should have a commit bit.  If you don't mind, I'll commend you to a committer and ask matz at the next developers' meeting.  What do you think?

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78058

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)
fix-readline-4.3-test-regression-v3.patch (1.33 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92702] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[merch-redmine]

Issue #15853 has been updated by jeremyevans0 (Jeremy Evans).


mame (Yusuke Endoh) wrote:
> @jeremyevans0 I think you should have a commit bit.  If you don't mind, I'll commend you to a committer and ask matz at the next developers' meeting.  What do you think?

I would be very honored to be a ruby committer. If matz approves, I will definitely accept a commit bit.  Thank you.

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78060

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)
fix-readline-4.3-test-regression-v3.patch (1.33 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92758] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[matz]

Issue #15853 has been updated by matz (Yukihiro Matsumoto).


@jeremyevans0 Welcome to the committers. 

Matz.

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78123

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)
fix-readline-4.3-test-regression-v3.patch (1.33 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92785] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[mame]

Issue #15853 has been updated by mame (Yusuke Endoh).


@jeremyevans0 Congrats!  Please wait for @hsbt's guidance.  (He will ask you to send your ssh key.)

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78151

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)
fix-readline-4.3-test-regression-v3.patch (1.33 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:92786] [Ruby trunk Bug#15853] Fix readline test regression when using Readline 4.3

[aycabta]

Issue #15853 has been updated by aycabta (aycabta .).


@jeremyevans0

Thanks a lot! I think that your v3 patch is correct...so please commit it yourself. Welcome.

----------------------------------------
Bug #15853: Fix readline test regression when using Readline 4.3
https://bugs.ruby-lang.org/issues/15853#change-78152

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: aycabta (aycabta .)
* Target version: 
* ruby -v: 
* Backport: 2.4: DONTNEED, 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
commit:c754e979d3eeca51f1b13778f19f347df3da656e removed the check for Readline 4.3 in a test.  Previously, the whole test was skipped on Readline 4.3.  However, it turns out that Readline 4.3 runs the test correctly if you skip the same assertion that is skipped when Reline is used.  The attached patch skips that assertion when `Readline::VERSION` is 4.3.

We may want to consider dropping this assertion completely, it seems to be readline implementation and version dependent behavior.

---Files--------------------------------
fix-readline-4.3-test-regression.patch (1.3 KB)
fix-readline-4.3-test-regression-v2.patch (1.28 KB)
fix-readline-4.3-test-regression-v3.patch (1.33 KB)


-- 
https://bugs.ruby-lang.org/