[RFC] Screen conceal highlight tests

[McKizzle]

Deleted the previous pull request (https://github.com/neovim/neovim/pull/2044)[https://github.com/neovim/neovim/pull/2044], added changes, created a feature branch, and fixed the commit message. Removed the search highlighting tests since (https://github.com/neovim/neovim/pull/1930)[https://github.com/neovim/neovim/pull/1930] already has several in place. @bfredl once your pull request is accepted I could add the extra highlighting tests into test/functional/ui/searchhl_spec.lua #1930 later.

[bfredl]

Nice work! Have you also considered testing e.g. concealends ?

[McKizzle]

Thanks, I'll clean that up. I've never used concealends before in Vim. I'll add tests for it after I learn how it works.

[bfredl]

Hmm, it looks like you rebased master commits into your feature branch, you probably want to the opposite (rebase your feature branch on top of master)
(Btw, not saying that that you need to test concealends as part of this PR, just pointing out that there are more options related to conceal that could be tested)

[McKizzle]

Sorry about that, this is my first time contributing to such a large repository. So you are saying that I should rebase my branch onto master and then rebase master onto the feature branch? Also, once a pull request is created is it recommended to keep it in sync with upstream?

[bfredl]

I am by no means a git expert, but I usually don't involve my local master branch at all when working on a feature branch, I do (on the feature branch)

git fetch origin
git rebase -i origin/master
make test #and/or test manually
git push # with -f if necessary

(where origin is the remote that refers to neovim/neovim )
There is usually no need to rebase a PR to master more frequently than when actual work is being done on the PR itself, (and usually not even then, unless the feature work is related to the changes on master, but it doesn't hurt to do it).

[McKizzle]

I'll do that. So I should probably recreate this pull request so that there are no side-effects?

Thanks!

[bfredl]

You can just force-push to the same branch (git push -f) and this PR will get updated, no need to create a new one.

[McKizzle]

That worked like a charm! 👍

[rainerborene]

Add a extra line above the it statement.

[bfredl]

Hmm, it seems like it is failing due to an actual redraw error (I can reproduce locally with my vimrc, but not with -u NONE )

[bfredl]

Actually nvim -u NONE and
:syntax on
:set conceallevel=1
6O&&<esc>
:syn match testmatch '[&][&]' conceal cchar=x
is enough to reproduce. (first x is not shown)

[bfredl]

seems issue in latest hg vim -u NONE -N as well, I will check/post to vim-dev

[bfredl]

@McKizzle Nice work with testing all the different options ! Please rebase on master when you have time, it should fix some incorrect failures on travis. Also the test needs to be updated as the character under the cursor is no longer hidden in the screen expectations, i e ^& is now ^&&.

[McKizzle]

Thanks! I'll get around to this later and update the tests to reflect the new screen expectations.

[McKizzle]

@bfredl changes have been pushed. I'll look around and see if there are any additional UI tests that I can add.

[bfredl]

These two describes could be joined. Perhaps a more descriptive header like describe("supports multiple matches", ?

[bfredl]

While tests are nice, the titles could be a bit more descriptive. Note that busted will concatenate the titles of nested describe/it block, so you could have (as an example)

describe("concealing", ...
  describe("supports multiple matches", ...
    it("and moving the cursor up", ...
  ...
  describe("handles conceallevel", ...
    it("2", ....

and busted will mention the individual test as "concealing supports multiple matches and moving the cursor up" and "concealing handles conceallevel 2" and so on.

[McKizzle]

@bfredl it would look cleaner that way. I'll combine those describes. Is there a testing style guide available on the wiki or in the docs? I did a search, but didn't find any.

[bfredl]

No, I have mostly followed earlier tests myself, as well as looking a bit in busted documentation, but perhaps a few lines should be added to the wiki somewhere (there is https://github.com/neovim/neovim/wiki/Unit-tests but it is about the "contents" of the tests and not the style)

[McKizzle]

OK.

[McKizzle]

Hello @bfredl. Sorry for the absence. Was in the process of getting a new job and moving to a new location. Is there a way to make busted print out the tests verbosely instead of as green dots? I noticed that busted lets you register your own output types, but I'm not sure how I would go about passing that in into TEST_FILE=test/functional/ui/syntax_conceal_spec.lua make unittest. That would make it easier to test the grammar.

[bfredl]

No hurry :) The simplest way, seems to be changing the line in CMakeLists.txt
set(BUSTED_OUTPUT_TYPE "utfTerminal")
to
set(BUSTED_OUTPUT_TYPE "gtest")
and rerun make test
anyway passing it as an environment variable directly to make test doesn't work, one have to invoke the busted command manually for that it seems.

[bfredl]

This also works (when standing in neovim, and already ran make test)
PATH=$PWD/.deps/usr/bin/:$PATH NVIM_PROG=build/bin/nvim ./.deps/usr/bin/busted -o gtest -v --lpath=build/\?.lua test/functional/ui/syntax_conceal_spec.lua

[McKizzle]

Thanks, I'll give that a try. That information is quite useful, I could add that to the Unit tests wiki page. Unless the Building Neovim page is a better location.

[McKizzle]

Using the other tests as a point of reference the new output looks like this.

Screen match and conceal multiple double ampersands
Screen match and conceal multiple double ampersands and move the cursor one line up.
Screen match and conceal multiple double ampersands and move the cursor to the beginning of the file.
Screen match and conceal multiple double ampersands and move the cursor to the second line in the file.
Screen match and conceal multiple double ampersands and then move the cursor to the beginning of the file and back to the end of the file.
Screen match and conceal keyword instances in initially in the document.
Screen match and conceal regions in the document initially and conceal it.
Screen match and conceal regions in the document initially and conceal its start tag and end tag.
Screen match and conceal regions in the document that are nested and conceal the nested region's start and end tags.
Screen match and conceal a region of text and turn on implicit concealing
Screen match and conceal a region of text and then turn on, then off, and then back on implicit concealing.
Screen let the conceal level be 0. No concealing.
Screen let the conceal level be 1. Conceal using cchar or reference listchars.
Screen let the conceal level be 2. Hidden unless cchar is set.
Screen let the conceal level be 3. Hide all concealed text.

[bfredl]

I think these, at least conceal_on and conceal_off, should be eliminated and the full command just put inside execute(). It is clearer if the command is just entered where it actually is used.

[McKizzle]

Will do that then.

[McKizzle]

I'll keep it consistent and remove the syn_region_i_text variable also.

[bfredl]

Nice! some additional comments, otherwise LGTM. 👍

If you want to do more screen tests (after this PR), it might be good to know that you can pass a screen size to Screen.new, like Screen.new(40, 10), to not get a bigger screen than actually needed. But there is no need to change this now in this PR (If you feel like it, a tip is to keep the width as 53 (default) and lower the lines from 14 to say, 10, and just delete 4 of the repeated lines in each except. But if there is some trouble just leave it like it is)

[McKizzle]

Thanks! Being able to modify the screen dimensions is helpful. I'll go through and minimize the height of the screens (they were making the file to large) and tidy up the code some more.

[bfredl]

Sorry, I was a bit busy with other stuff and forgot about this PR. Please rebase and squash.

[McKizzle]

No problem! Rebased and squashed the branch.

[bfredl]

Merged, thanks!

[McKizzle]

Yay! Should I close this branch now?

[bfredl]

Yes, you can delete the branch.
Den 9 maj 2015 5:41 em skrev "Clinton McKay" notifications@github.com:

Yay! Should I close this branch now?

—
Reply to this email directly or view it on GitHub
#2055 (comment).