[syntax] How can pre- and post-setup for screen dump generation be achieved?

[zzzyxwvut]

Grepping for exists( in runtime/syntax and looking at
the matched results reveals that it is common to have some
conditional recognition functionality for parts of syntax:

cd runtime/syntax
vim +/exists\( $(grep -c exists\( *.vim | grep :[2-9][0-9] | cut -d: -f1)

According to runtime/syntax/Makefile, Vim is launched with
the --clean flag (and so runtime/defaults.vim is used for
vimrc). Currently, it seems, the very same syntax file is
the only place where a syntax file maintainer or contributor
can define global variables (and remove them when needed),
so that the file parts guarded with such variables can be
read during screen dump generation. This approach may lead
to littering the syntax file with test-related queries that
tentatively compare the current buffer name and a filename
from runtime/syntax/testdir/input, look for any clues in
the current working directory, etc.

Can we do better?

[zzzyxwvut]

So, I've put together this prototype.

@chrisbra, would care to have a look at it?

[dkearns]

Thanks, @zzzyxwvut. Sorry, I don't always seem to get notifications so I missed this one.

There is already a comment-based mechanism for test setup in the indent test runner. That could probably be reused here as well.

[zzzyxwvut]

All right, here is an alternative prototype based on
INDENT_EXE from indent/testdir/runtest.vim:

vim/runtime/indent/testdir/runtest.vim

Lines 64 to 68 in 5131f22

	let lnum_exe = search('\<INDENT_EXE\>')
	exe lnum + 1
	let indent_at = search('\<INDENT_\(AT\|NEXT\|PREV\)\>')
	if lnum_exe > 0 && lnum_exe < end && (indent_at <= 0 || lnum_exe < indent_at)
	exe substitute(getline(lnum_exe), '.*INDENT_EXE', '', '')

Some observation regarding the newer prototype, so far.

Pros:

• There is no need to maintain a set of configuration files;

• There is one less file to source for Vim;

• Implementation is straightforward.

Cons:

• Lines will be scanned for SYNTAX_EXE for every input/
  file (it can be limited to, say, 20 top lines);

• The syntax-related setup is doubled for a test file with
  SYNTAX_EXE "commands";

• Files under syntax/testdir/input are written in a way,
  it is hoped, that they are well-formed for interpretation
  or compilation by their own language tools; in order to
  keep that promise, comments, if possible, would be used
  for SYNTAX_EXE "commands"; otherwise strings or plain
  text would be resorted to (Markdown, SubRip, etc.);

• There is no telling what is the meaning of SYNTAX_EXE, or
  any arbitrary word, for a language at large outside of
  a comment or a string (therefore, any namesake keywords,
  commands, etc., must not be written within, say, 20 top
  lines).

[dkearns]

I was thinking the SYNTAX_EXE lines could be stripped before the test is run but it's still unpleasant to have to work with test files where the syntax highlighting is broken in their static state.

It looks like about 90% of the current syntax filetypes allow comments. Most of those that don't are very simple and will ignore the config lines, however, there will always be exceptions that probably also require the most testing. Why not offer both methods?

The syntax-related setup is doubled for a test file with SYNTAX_EXE "commands";

I don't understand this comment. Could you elaborate?

Thanks for working on this.

[zzzyxwvut]

As the second prototype currently implemented, there will be
:edit foo.bar (for which :syntax on will be applied from
$VIMRUNTIME/defaults.vim, and eventually syntax/bar.vim
will be sourced), followed by the searching and processing
of found SYNTAX_EXE, and concluded by :doautocmd Syntax
(or any its alternative). Hence, the double sourcing of
syntax/bar.vim and any :syn-include files it may refer to,
recursively.

Here, is the marriage of two prototypes (with documentation
drafted).

[dkearns]

Is there a reason not to start with -Nu NONE and enable syntax once after all the config has been executed?

[zzzyxwvut]

I doubt it. Let's postpone the sourcing of defaults.vim.
Please see an updated version of the 3rd prototype.

[zzzyxwvut]

A commentless Markdown test file can now be processed with
the prototype.

[zzzyxwvut]

Now that the benefit of offering a support for embeddable
and sourceable Ex commands is clearly understood, wouldn't
their complementary functionality be better expressed by
choosing SYNTAX_BEFORE_EXE over SYNTAX_EXE to imply
runtime/syntax/testdir/input/before? As we haven't touched
the subject of tear-down operations and a need for it, for
the present, is not well defined or further pursuable on my
part (context: a new instance of Vim is launched for each
runtime/syntax/testdir/input test file), but should it be
entertained in the future, wouldn't we prefer to have these
labels paired like *BEFORE* and *AFTER*?

There are no matches of SYNTAX_{,{BEFORE,AFTER}_}EXE in the
test files under runtime/syntax/testdir/input in their
master versions, so an introduction of the support for
embeddable Ex commands wouldn't require any assistance from
test file maintainers.

[chrisbra]

Not sure I understand.
I suppose SYNTAX_EXE could potentially be used like SYNTAX_BEFORE_EXE and SYNTAX_AFTER_EXE. It's just not that clear. I don't really have a preference for one over the other.

[zzzyxwvut]

The latest hybrid prototype version has only SYNTAX_EXE.
That name has been chosen to resemble the name INDENT_EXE
(from indent/testdir/runtest.vim) for a prototype that would
only support embeddable (in comments) Ex commands. Later, it
turned out that having two prototypes (another one being Vim-
file sourceable) rolled into one can prove a better
alternative, for there are languages with no support for
comments. Now, I'm asking whether it would fare better to
replace SYNTAX_EXE with SYNTAX_BEFORE_EXE (it may look more
coherent in runtime/syntax/testdir/README.txt when it is
discussed alongside with runtime/syntax/testdir/before).
And we can have a SYNTAX_AFTER_EXE name "reserved" should
ever need it in the future.

What we wouldn't want to have is to maintain three names
some time later instead of two, and having *BEFORE* serve
as a façade for the functionality of SYNTAX_EXE.

Otherwise, I'm fine with either name, like you are.

[dkearns]

I don't think adding BEFORE and AFTER variants at a later stage, while keeping SYNTAX_EXE for compatibility, is that much of a concern.

{SYNTAX,INDENT}_EXE don't strike me as great names. I think something with setup in the name would be clearer than before and *_EXE. E.g., testdir/setup/ and TEST_SETUP

[zzzyxwvut]

That is a sound compromise. Let's go with setup/ and
TEST_SETUP.

[zzzyxwvut]

Okay. The commit that introduces the hybrid support has been
teased out into two separate commits for each prototype. The
history ought to be now more review-friendly.

[dkearns]

I added a couple of comments, thanks.

[zzzyxwvut]

Another round of changes has been pushed.

[dkearns]

I added a few more comments. I think this looks good, can you please make a PR for this?

[zzzyxwvut]

Apropos of PRs, with a universal convention of squashing
multi-commit PRs in this repo and the nature of the changes
at hand being a two distinct ways of solving one problem,
would it be more convenient to present them as separate PRs,
with the TEST_SETUP changes going in first and the setup/
changes building upon it, for the benefit of future
modification?

[chrisbra]

I tend to squash multiple commits, if they all handle the same "topic", but if they handle separate issues within a single PR, I'll merge them separately.

[zzzyxwvut]

Sorry for the generalisation and thank you for clarifying
your approach.

[dkearns]

would it be more convenient to present them as separate PRs,
with the TEST_SETUP changes going in first and the setup/
changes building upon it, for the benefit of future
modification?

I don't think it needs to be split into two but if you're more comfortable with that it's fine by me.

[dkearns]

Is LoadFiletype() really necessary? I think the syntax files should be tested in isolation from the ftplugins. If they're needed for a specific test then they can be sourced in the test's setup.

[zzzyxwvut]

There is another plot, related to screen-dump generation,
going on. Please see the comments in these PRs: #14105,
#14120, #14150.

[zzzyxwvut]

With defaults.vim out of the way, we would have to assemble
and maintain a list of Vim options that are changed from the
defaults in that file and bear relation to testing. Without
it, some dumps/* files would differ from generated failed/*
files.

So far the list is:

	  " BEGIN [runtime/defaults.vim]
	  set display=truncate ruler scrolloff=5
	  " Provide pre-TEST_SETUP support for input/c.c.
	  let g:c_comment_strings = 1
	  syntax on
	  " END [runtime/defaults.vim]

I'm not sure about &showcmd, &ttimeout, &ttimeoutlen.

Note that we get &nocompatible from -N.

It's weird, but the :syntax on machinery seems to recognise
global variables as such only when those bear a g: prefix
with make clean test.

This legacy script alternative is ignored:

	  let c_comment_strings = 1

But whether g: is used or not makes no difference when
a file is opened by hand, e.g.:

../../src/vim --clean

(The Ex commands replicate how RunVimInTerminal() goes about
its business.)

:split | vsplit | set t_Co=256 background=light | hi Normal ctermfg=NONE ctermbg=NONE
:let g:vim_id = term_start('vim -f --clean --gui-dialog-file guidialogfile --cmd "set enc=utf8" -v --cmd "set t_u7=" -Nu NONE', {'term_cols': 75, 'curwin': 1, 'term_rows': 20, 'term_name': printf('[PPID:%d]', getpid())})

:edit testdir/input/c.c
:set display=truncate ruler scrolloff=5
:let c_comment_strings = 1
:syntax on

[chrisbra]

If the changes to the test-suite requires changing all the dump files, that's not nice, but so be it. 🤷

[zzzyxwvut]

May I suggest that we settle for not using defaults.vim,
but keep these lines in SetUpVim(), for now:

	  " BEGIN [runtime/defaults.vim]
	  set display=truncate ruler scrolloff=5
	  syntax on
	  " END [runtime/defaults.vim]

Apply the change (see the above comment) to input/c.c,
because the dumps/* files for that file would be broken
without g:c_comment_strings declared.

Then work elsewhere on the line discontinuation problem for
test files with lines over 75 columns. And while at it, see
if &display can be used with its default value for it.

And only then decide whether to keep these three options
with non-default values or some of them or get rid of all.

[dkearns]

Sounds fine to me.

[zzzyxwvut]

Good. Then I'll present three commits as the final WIP changes:

1. The embedabble setup support (with the changes touching
   defaults.vim squashed in).
2. The sourceable setup support.
3. Two examples of usage: c.c and markdown_conceal.markdown.

[zzzyxwvut]

I have filed an issue for the line discontinuation problem.

[zzzyxwvut]

The WIP changes are up.

There was another :syntax on at the top of Xtestscript!!
But now its use seems redundant, so it's gone.

Is there a need to protect introduced line continuations
with :set cpoptions-=C?

[dkearns]

No, the test script is run with --clean which handles that.

[zzzyxwvut]

The final WIP changes are up.

If all is well, I can draft a PR tomorrow.

[chrisbra]

looks good. There is one comment that is hard to understand however:

Remember that only for Vim configuration can "TEST_SETUP" appear within the top 20 lines

[zzzyxwvut]

How about I expand it to this:

Remember that no heuristic is set in place to discover whether the matched
lines belong to a comment or any other valid or invalid construct of the
language under test; therefore, only for Vim configuration can "TEST_SETUP"
appear within the top 20 lines.

[zzzyxwvut]

Basically, if you use some, say, C-like macro TEST_SETUP
within the top 20 lines, the part between it and the end of
line would be "happily" :executed by Vim.

Your options are:

• Rename it.
• Keep the name, but use it below the top 20 lines.

[zzzyxwvut]

To follow up our discussion regarding whether to prefer non-
default or default values for some Vim options, it turns out
that &ruler is heavily used in input/sh_*.sh tests, with
a dedicated function Xtestscript#ShellInfo() declared in
runtest.vim to lend a hand and cater for self-discovery of
shell script identity under test.

For convenience, these are current &ruler prefix contents:

sh_01_00.dump: is_dash: 1, is_posix: 1, is_sh: 1,
sh_02_00.dump: is_kornshell: 1,
sh_03_00.dump: is_kornshell: 1,
sh_04_00.dump: is_dash: 1, is_posix: 1, is_sh: 1,
sh_05_00.dump: is_dash: 1, is_posix: 1, is_sh: 1,
sh_06_00.dump: is_kornshell: 1,
sh_07_00.dump: is_dash: 1, is_posix: 1, is_sh: 1,
sh_08_00.dump: is_kornshell: 1,
sh_09_00.dump: is_dash: 1, is_posix: 1, is_sh: 1,
sh_10_00.dump: is_kornshell: 1,
sh_11_00.dump: is_bash: 1,

I think this is a use case for potential input/teardown/ and
VIM_TEST_TEARDOWN.

With other teardown applications that come to mind:

• To verify whether :syn-include have worked (hlexists()).

• To assert whether the correct syntax file was sourced for
  a test file that shares its file extension among a few
  programming languages, e.g. *.conf ( b:current_syntax).

If these shell-related assertions can be rewritten, &ruler
and &scrolloff can probably go back to their defaults. But
I shudder to think that it would demand regenerating 300
dumps/ files and diligent comparing of their contents with
their old versions.

---

The current &ruler prefix contents for sh_01_99.dump and
sh_09_99.dump are :redraw. This output comes from these
lines:

vim/runtime/syntax/testdir/runtest.vim

Lines 204 to 207 in 70e566b

	" clear the shell info if there are not enough lines to cause a scroll
	if filetype == 'sh' && linecount <= 19
	call term_sendkeys(buf, ":redraw\<CR>")
	endif

Shouldn't the :redraw line rather be changed to:

	call term_sendkeys(buf, ":redraw!\<CR>")

?

---

As for &display, changing its current truncate value to
lastaline (neither is default) may be more advantageous if
only to sooner discover whether any lines are missing.