Use utf-8 encoding for child Vim in Test_terminal_dumpwrite_composing by jamessan · Pull Request #2847 · vim/vim

jamessan · 2018-04-26T13:56:14Z

The test is relying on the child Vim process to display the composed
characters a certain way in order to test term_dumpwrite(). However, if
the tests aren't running in a locale that defaults the child Vim's
'encoding' to utf-8, then the text will look different than expected
(e.g. a~@ e~B o~H).

Forcing the child Vim to use encoding=utf-8, just like the main Vim is
doing, fixes the issue.

The test is relying on the child Vim process to display the composed characters a certain way in order to test term_dumpwrite(). However, if the tests aren't running in a locale that defaults the child Vim's 'encoding' to utf-8, then the text will look different than expected (e.g. "a~@ e~B o~H"). Forcing the child Vim to use encoding=utf-8, just like the main Vim is doing, fixes the issue.

codecov-io · 2018-04-26T14:16:02Z

Codecov Report

Merging #2847 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2847      +/-   ##
==========================================
- Coverage    75.4%   75.39%   -0.01%     
==========================================
  Files          92       92              
  Lines      134738   134738              
==========================================
- Hits       101593   101592       -1     
- Misses      33145    33146       +1

Impacted Files	Coverage Δ
src/if_xcmdsrv.c	`83.63% <0%> (-0.36%)`	⬇️
src/ex_cmds.c	`79.38% <0%> (-0.09%)`	⬇️
src/window.c	`81.89% <0%> (-0.07%)`	⬇️
src/term.c	`60.44% <0%> (-0.06%)`	⬇️
src/gui.c	`47.78% <0%> (-0.06%)`	⬇️
src/gui_gtk_x11.c	`47.73% <0%> (+0.09%)`	⬆️
src/channel.c	`83.14% <0%> (+0.11%)`	⬆️
src/os_unix.c	`54.48% <0%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db0eede...61c37c0. Read the comment docs.

jamessan · 2018-04-27T12:36:40Z

As an aside, it wasn't very easy to see why this failed from a test log perspective. While I was trying to figure this out (since it was only replicating in automated builds), I used the following patch to get more information:

diff --git i/src/testdir/test_terminal.vim w/src/testdir/test_terminal.vim
index 1b87895ac..bf31de9de 100644
--- i/src/testdir/test_terminal.vim
+++ w/src/testdir/test_terminal.vim
@@ -1026,7 +1026,11 @@ func Test_terminal_dumpwrite_composing()
   let text = " a\u0300 e\u0302 o\u0308"
   call writefile([text], 'Xcomposing')
   let buf = RunVimInTerminal('Xcomposing', {})
-  call WaitFor({-> term_getline(buf, 1) =~ text})
+  try
+    call WaitFor({-> term_getline(buf, 1) =~ text})
+  catch /WaitFor/
+    call assert_match(text, term_getline(buf, 1))
+  endtry
   call term_dumpwrite(buf, 'Xdump')
   let dumpline = readfile('Xdump')[0]
   call assert_match('|à| |ê| |ö', dumpline)

However, doing that everywhere is a lot of repetition. If the assert_* functions returned distinct values when the assert passes or fails, then it would be possible to directly use that in the WaitFor. Something like:

diff --git i/src/testdir/shared.vim w/src/testdir/shared.vim
index 6c3f8a4e6..6c8ac1f7b 100644
--- i/src/testdir/shared.vim
+++ w/src/testdir/shared.vim
@@ -132,7 +132,9 @@ func WaitFor(expr, ...)
   else
     let Test = {-> eval(a:expr) }
   endif
+  let errors = copy(v:errors)
   for i in range(timeout / 10)
+    let v:errors = errors
     if Test()
       if has('reltime')
 	return float2nr(reltimefloat(reltime(start)) * 1000)
diff --git i/src/testdir/test_terminal.vim w/src/testdir/test_terminal.vim
index 1b87895ac..e1aba9508 100644
--- i/src/testdir/test_terminal.vim
+++ w/src/testdir/test_terminal.vim
@@ -1026,7 +1026,7 @@ func Test_terminal_dumpwrite_composing()
   let text = " a\u0300 e\u0302 o\u0308"
   call writefile([text], 'Xcomposing')
   let buf = RunVimInTerminal('Xcomposing', {})
-  call WaitFor({-> term_getline(buf, 1) =~ text})
+  call WaitFor({-> assert_match(text, term_getline(buf, 1))})
   call term_dumpwrite(buf, 'Xdump')
   let dumpline = readfile('Xdump')[0]
   call assert_match('|à| |ê| |ö', dumpline)

brammool · 2018-04-27T21:02:11Z

James McCoy wrote:

As an aside, it wasn't very easy to see _why_ this failed from a test log perspective. While I was trying to figure this out (since it was only replicating in automated builds), I used the following patch to get more information: ```diff diff --git i/src/testdir/test_terminal.vim w/src/testdir/test_terminal.vim index 1b87895ac..bf31de9de 100644 --- i/src/testdir/test_terminal.vim +++ w/src/testdir/test_terminal.vim @@ -1026,7 +1026,11 @@ func Test_terminal_dumpwrite_composing() let text = " a\u0300 e\u0302 o\u0308" call writefile([text], 'Xcomposing') let buf = RunVimInTerminal('Xcomposing', {}) - call WaitFor({-> term_getline(buf, 1) =~ text}) + try + call WaitFor({-> term_getline(buf, 1) =~ text}) + catch /WaitFor/ + call assert_match(text, term_getline(buf, 1)) + endtry call term_dumpwrite(buf, 'Xdump') let dumpline = readfile('Xdump')[0] call assert_match('|à| |ê| |ö', dumpline) ``` However, doing that everywhere is a lot of repetition. If the `assert_*` functions returned distinct values when the assert passes or fails, then it would be possible to directly use that in the `WaitFor`. Something like: ```diff diff --git i/src/testdir/shared.vim w/src/testdir/shared.vim index 6c3f8a4e6..6c8ac1f7b 100644 --- i/src/testdir/shared.vim +++ w/src/testdir/shared.vim @@ -132,7 +132,11 @@ func WaitFor(expr, ...) else let Test = {-> eval(a:expr) } endif + let errors = copy(v:errors) for i in range(timeout / 10) + if v:errors != errors: + let v:errors = errors + endif if Test() if has('reltime') return float2nr(reltimefloat(reltime(start)) * 1000) diff --git i/src/testdir/test_terminal.vim w/src/testdir/test_terminal.vim index 1b87895ac..e1aba9508 100644 --- i/src/testdir/test_terminal.vim +++ w/src/testdir/test_terminal.vim @@ -1026,7 +1026,7 @@ func Test_terminal_dumpwrite_composing() let text = " a\u0300 e\u0302 o\u0308" call writefile([text], 'Xcomposing') let buf = RunVimInTerminal('Xcomposing', {}) - call WaitFor({-> term_getline(buf, 1) =~ text}) + call WaitFor({-> assert_match(text, term_getline(buf, 1))}) call term_dumpwrite(buf, 'Xdump') let dumpline = readfile('Xdump')[0] call assert_match('|à| |ê| |ö', dumpline) ```

Ignoring an error from the function might hide a mistake. The assert functions are not really setup for just checking. Would require some "try but don't throw" argument, which doesn't make it simpler. We could add WaitForMatch(function, pattern), that would cover the most common cases. Or an argument to WaitFor() to not throw an exception but return, so the assert can be after it (like we had earlier).

…

-- Facepalm statement #6: "Estland is a fantasy place, just like Middle Earth and Madagaskar" /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

jamessan · 2018-04-27T23:06:32Z

Ignoring an error from the function might hide a mistake.

The whole point of WaitFor is that the condition is expected to fail initially, so there's currently no reason to use an assert function inside. Anything that's using assert_* inside of a WaitFor() right now would be causing false positive test failures.

The assert functions are not really setup for just checking. Would require some "try but don't throw" argument, which doesn't make it simpler.

The assert functions don't throw. They simply add to v:errors when they fail. Enhancing them to return whether they succeed or fail would help in this case.

Or an argument to WaitFor() to not throw an exception but return, so the assert can be after it (like we had earlier).

There are currently a lot of calls that look like:

call WaitFor({-> check condition})
" Use an assert_*() to check the same condition

This code is useless because WaitFor() throws. The following assert would never be called, which means we lose diagnostic information.

It would be more convenient to allow WaitFor() to use assert_* so it can actually report whatever caused the condition to fail, instead of repeating checks.

Given the above, I think letting assert_*()'s return value return whether the condition was true or not would be the simplest solution and allow us to make WaitFor() more useful.

brammool · 2018-04-28T12:51:15Z

James McCoy wrote:

> Ignoring an error from the function might hide a mistake. The whole point of WaitFor is that the condition is _expected_ to fail initially, so there's currently no reason to use an assert function inside. Anything that's using `assert_*` inside of a `WaitFor()` right now would be causing false positive test failures. > The assert functions are not really setup for just checking. Would require some "try but don't throw" argument, which doesn't make it simpler. The assert functions don't throw. They simply add to `v:errors` when they fail. Enhancing them to return whether they succeed or fail would help in this case. > Or an argument to WaitFor() to not throw an exception but return, so the assert can be after it (like we had earlier). There are currently a lot of calls that look like: ```vim call WaitFor({-> check condition}) " Use an assert_*() to check the same condition ``` This code is useless because `WaitFor()` throws. The following assert would never be called, which means we lose diagnostic information.

That's a leftover from when WaitFor() didn't throw an exception. These should be removed.

It would be more convenient to allow `WaitFor()` to use `assert_*` so it can actually report whatever caused the condition to fail, instead of repeating checks. Given the above, I think letting `assert_*()`'s return value return whether the condition was true or not would be the simplest solution and allow us to make `WaitFor()` more useful.

Hmm, OK. Some extra work is done to add an item to v:errors, which we would then remove again. But that's not so bad. An alternative would be to add an argument to the assert_ functions to only check and not add to v:errors, but these functions already have various arguments, thus that's not making it simpler. We can make it so that when the assert_ function fails it always adds exactly one entry to v:errors and returns non-zero.

…

-- Microsoft's definition of a boolean: TRUE, FALSE, MAYBE "Embrace and extend"...? /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

brammool · 2018-04-28T19:36:37Z

I wrote:

James McCoy wrote: > > Ignoring an error from the function might hide a mistake. > > The whole point of WaitFor is that the condition is _expected_ to fail > initially, so there's currently no reason to use an assert function > inside. Anything that's using `assert_*` inside of a `WaitFor()` > right now would be causing false positive test failures. > > > The assert functions are not really setup for just checking. Would require some "try but don't throw" argument, which doesn't make it simpler. > > The assert functions don't throw. They simply add to `v:errors` when > they fail. Enhancing them to return whether they succeed or fail > would help in this case. > > > Or an argument to WaitFor() to not throw an exception but return, so the assert can be after it (like we had earlier). > > There are currently a lot of calls that look like: > > ```vim > call WaitFor({-> check condition}) > " Use an assert_*() to check the same condition > ``` > > This code is useless because `WaitFor()` throws. The following assert > would never be called, which means we lose diagnostic information. That's a leftover from when WaitFor() didn't throw an exception. These should be removed. > It would be more convenient to allow `WaitFor()` to use `assert_*` so > it can actually report whatever caused the condition to fail, instead > of repeating checks. > > Given the above, I think letting `assert_*()`'s return value return > whether the condition was true or not would be the simplest solution > and allow us to make `WaitFor()` more useful. Hmm, OK. Some extra work is done to add an item to v:errors, which we would then remove again. But that's not so bad. An alternative would be to add an argument to the assert_ functions to only check and not add to v:errors, but these functions already have various arguments, thus that's not making it simpler. We can make it so that when the assert_ function fails it always adds exactly one entry to v:errors and returns non-zero.

With patch 8.0.1771 I made it work this way. I changed about half the uses of WaitFor(). Does this look OK?

…

-- GALAHAD turns back. We see from his POV the lovely ZOOT standing by him smiling enchantingly and a number of equally delectable GIRLIES draped around in the seductively poulticed room. They look at him smilingly and wave. "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

jamessan · 2018-04-30T01:23:01Z

With patch 8.0.1771 I made it work this way. I changed about half the uses of WaitFor(). Does this look OK?

I'll test it out. Do you have any comment on the original issue in this PR? :) I know I side-tracked the discussion a little bit with the WaitFor stuff.

brammool · 2018-04-30T07:58:51Z

James McCoy wrote:

> With patch 8.0.1771 I made it work this way. I changed about half the uses of WaitFor(). Does this look OK? I'll test it out. Do you have any comment on the original issue in this PR? :) I know I side-tracked the discussion a little bit with the WaitFor stuff.

It's near the top of the todo list now. May I'll finish the WaitFor change, otherwise I'll forget.

…

-- TALL KNIGHT OF NI: Ni! "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

…ding' Problem: Test fails because Vim in a terminal uses wrong 'encoding'. Solution: Set encoding in the test where it matters. (James McCoy, closes vim#2847)

brammool closed this in 77bfd75 Apr 30, 2018

jamessan deleted the dumpwrite_composing-encoding branch June 1, 2018 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use utf-8 encoding for child Vim in Test_terminal_dumpwrite_composing - #2847

Use utf-8 encoding for child Vim in Test_terminal_dumpwrite_composing#2847
jamessan wants to merge 1 commit into
vim:masterfrom
jamessan:dumpwrite_composing-encoding

jamessan commented Apr 26, 2018 •

edited

Loading

Uh oh!

codecov-io commented Apr 26, 2018

Uh oh!

jamessan commented Apr 27, 2018 •

edited

Loading

Uh oh!

brammool commented Apr 27, 2018 via email

Uh oh!

jamessan commented Apr 27, 2018

Uh oh!

brammool commented Apr 28, 2018 via email

Uh oh!

brammool commented Apr 28, 2018 via email

Uh oh!

jamessan commented Apr 30, 2018

Uh oh!

brammool commented Apr 30, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

jamessan commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 26, 2018

Codecov Report

Uh oh!

jamessan commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brammool commented Apr 27, 2018 via email

Uh oh!

jamessan commented Apr 27, 2018

Uh oh!

brammool commented Apr 28, 2018 via email

Uh oh!

brammool commented Apr 28, 2018 via email

Uh oh!

jamessan commented Apr 30, 2018

Uh oh!

brammool commented Apr 30, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamessan commented Apr 26, 2018 •

edited

Loading

jamessan commented Apr 27, 2018 •

edited

Loading