2018-02-19 23:06 GMT+03:00 Bram Moolenaar <b...@moolenaar.net>:
> Nikolay Pavlov wrote:
>> Some notes which I think are not clear from my rather lengthy (and
>> probably too angry) post:
>> 1. Point 1 and 2 are just something that is worth writing VimL library
>> for. Would not suggest to solve them in C.
>> 2. Have not wrote that, but I expect point 5 to be fixed in the
>> future, after feature is stabilized.
>> 3. Points 3 (where screen state should be kept) and 4 (in what format)
>> are actual complains about suboptimal design.
> [and more in an earlier first message]
> Keep in mind that this screen dump feature is like creating a screenshot
> in a .png file.  Nobody is expected to edit the output, you should use
> the term_dumpwrite() function to create them and term_dumpload() to view
> them.
> In my experience making a screenshot is quick and efficient, you just
> need to make sure to get to the right state before making the
> screenshot.  Dealing with test failures should be a lot easier visually
> than trying to look at a text description of the expected output (and
> adjusting the text, instead of accepting the new file if it looks OK).
> I intentionally put this in a file and not as text inside a test script.
> I don't think a test script is the right place, it will get in the way
> of the actual test code.  It would also require the developer to
> copy/paste text, since you would never write the expected output, only
> generate it.  Again, use it like a screenshot.
> If you do want to check text you can already do that with getline().  If
> you want to check highlighting you can use synIDattr() in most cases. It
> gets more tricky for things like completion and popups, that's where the
> dump feature comes in.
> I did intentionally use a text format and not a binary format.  Partly
> to make it easier to make patches and send them around. Partly to allow
> me to debug the code for writing and loading these files.  But they are
> not intended to be human readable or editable.  I rather keep them
> small, since I expect them to be used a lot.  A few examples I tried
> were less than 5 Kbyte, which should be fine.

The “text screenshots” in the format Neovim uses are readable by
humans, though less then actual screenshots. New dump files are not,
and you can view only one file at a time and only with a hack
(`term_dumpdiff('test.tdump', '/dev/null')`). When something in test
changes you can use actual diffs to view what exactly changed (though
Neovim does not, 20 x 75 screen would be better with diffs, but
smaller screens Neovim uses are fine without them) and you get the
readable diff directly in test log. Can’t imagine putting current dump
diffs in test log in any readable format: `diff expected.tdump
actual.tdump` is not readable if text or some positions changed,
pasting `term_dumpdiff` buffer (or even viewing it interactively in
Vim) may be not very helpful in case of some format changes without
changing texts (e.g. you would not find any visual difference between
0xFEFFFF and 0xFFFFFF colors even if monitor is actually capable of
displaying this difference).

Yes, there is many copy-pasting: sometimes from tests to tests,
sometimes from `screen:snapshot_util()` output to the test log. But
tests are copy-pasted a lot in any case and you *can* do copy-paste
with our format. Can’t see why it needs to be prevented. Imagine the
test needs to go through a bunch of screen states and check them all.
In Neovim I write this as

    # Some action 1
    # Some action 2
    # Some action 3

Then I *review all snapshot_util() outputs **in one go***, copy them
to a proper location without any modifications (Vim is nice enough to
have `]p`), remove `snapshot_util()` calls, commit and am done.

In your case I can still put `term_dumpwrite()` calls, but difference
starts even here: I need new unique names to each of screen states and
everybody knows that naming is hard. Especially if I can’t name them
`{test_file_name}_{test_name}_frame{N}` without getting problems next
time I realize that I need to see screen state between frames 1 and 2
(also another thing to keep in mind for contributors: what is the
frame naming convention). But even after I solve naming problem there
are more actions:

1. I need to review frames *one by one* because they are not readable
even if I `cat` them. It is worse if the screen `snapshot_util` is
used for examining screen states rather then creating actual tests,
the variant where screen state is examined in a running Vim instance
is worse for a number of reasons (for one, tests are supposed to
finish and to do this you would need to make something for them to not
finish and show you screen state and after finishing examination you
would need to undo what you did; another reason is that examining a
*sequence* of frames like this is even harder to do; third is that I
can’t imagine it fast and I especially can’t imagine it fast if I
really do want to check that some text has 0xFEFFFF color and another
2. Then in place of just copy-pasting completely ready text I need to
now replace `term_dumpwrite()` calls with something writing to
temporary file (why I can’t create a variable instead? It adds another
object to the set of things I need to know to test Vim: how temporary
file needs to be named to not cause problems with other tests and to
make it cleaned up by `make clean`).
3. Then I need to add `term_dumpdiff()` (most likely, some wrapper
rather then actual `term_dumpdiff()`) call pointing it to a created
frame and temporary file.
4. Then I need to not forget to `git add` those files. Could remind
you that forgetting to `git add` some new files is one of the most
common cases of Vim follow-up “bug fix” commits, good they are easy to
find and are mostly fixed at the same day, and definitely before going
into distribution packages.
5. Only then I could commit the test.

Do not forget another thing: if frame is renamed or test removed it is
too easy to leave garbage behind. If frame is put into test source
code you need no names and you can’t remove a test and not remove a
“screenshot”. On your place I would not like creating a linter to fix
the problem.


BTW, proposed format is also *smaller*.

> As mentioned, I haven't written a test with this dump feature yet.  I do
> intend to use Vim script for that, we'll see if that works out.  I hope
> to have time tomorrow.  Hmm, we should probably include the cursor
> position in the dump somehow.
> Since for tests the minimum terminal size is 24 x 80 characters, we need
> to use dumps that are a bit smaller than that.  Possibly 20 x 75
> characters.  Then it will work everywhere. That should be enough for
> simple tests, but not for checking a whole file for syntax highlight
> errors.  Perhaps we can find a way to do that later.
> The max-width and max-height options are for taking a dump of part of
> the screen, not for setting the terminal size.  Maybe an offset would
> also be useful.  But none of this is implemented yet.  Oh, I forgot to
> adjust the help for this, it should have an {options} argument like the
> otheres. Otherwise adding more choices is going to make it ugly.
> For "waiting for a specific state": we already have term_wait(), but
> it's not very reliable.  The WaitFor() function is more useful, and we
> can use something like that, waiting the file contents to be equal.
> Waiting for an intermediate state is not a good idea, it most often
> leads to flaky tests.
> Viewing the diff in three parts then does require a terminal that is
> three times higher, to be able to see the whole.  Actually, one only
> needs to see two parts, since the "s" key swaps the top and bottom.
> This blinking will show the difference better anyway.
> If the dumps are 20 lines then most developers will be able to see the
> three parts, or at least two.  If we do add a way to make larger dumps
> that will fail.  A vertical split could then help, if your terminal is
> wide enough (more than 150 if we stick to the 75 char width).
>> 4. And the whole post is written from the POV of a person thinking
>> “Bram again wrote suboptimal feature without asking anybody (or Neovim
>> developers specifically) regarding how they solve the issue”. Why? We
>> do have similar problems, we are not against sharing experience and
>> asking *before* writing feature would make design better from the
>> beginning.
> I do not recall any Neovim developers discussing any of their testing
> stuff in the Vim developer list.  I don't see why I would ask for
> comments before implementing anything, unless I don't know what to
> implement.  In this case I have a very good idea of what to do, since
> I'm using similar tools inside Google.  And if there are good
> suggestions there is no problem using them.

I can’t remember discussions about specifically testing stuff, but it
is not the first reimplementation of something Neovim already has, and
it is not the first implementation which was first merged and then
discussed by (or without in some cases) request.

> --
> In Africa some of the native tribes have a custom of beating the ground
> with clubs and uttering spine chilling cries.  Anthropologists call
> this a form of primitive self-expression.  In America we call it golf.
>  /// Bram Moolenaar -- b...@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    ///

You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

You received this message because you are subscribed to the Google Groups 
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to vim_dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Raspunde prin e-mail lui