Skip to content

Add a few tests #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 19, 2022
Merged

Add a few tests #43

merged 2 commits into from
May 19, 2022

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented Apr 28, 2022

As a bit of a follow-up to #42, this adds tests for the syntax
highlighting.

Example zsh files are in testdata/*.zsh; after writing test files are
verifying it's all good you can generate with "make update-tests", and
you can run it with "make test". I included the testing.vim as a
submodule here.

As a bit of a follow-up to chrisbra#42, this adds tests for the syntax
highlighting.

Example zsh files are in testdata/*.zsh; after writing test files are
verifying it's all good you can generate with "make update-tests", and
you can run it with "make test". I included the testing.vim as a
submodule here.
Copy link
Contributor

@danielshahaf danielshahaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thumbs up for adding automated testing. Haven't done a full review; just pointing out a few issues I noticed in my read-through.

testdata/if.zsh Outdated
Comment on lines 25 to 26
# TODO: maybe show error for these invalid constructs (missing spaces)?
[[x=x]] && [x=x]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't invalid constructs; they're perfectly valid glob patterns. The first looks for a file named [] or =] or x]. The second one looks for a file named x or =.

Nevertheless, I agree that it might make sense to highlight these constructs as "this is probably wrong" (compare zsh-users/zsh-syntax-highlighting#695 and the issue linked from the first post there).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, of course. I think we can get around that by highlighting it as an error only if it's after a zshConditional, or if it's preceded or followed by zshOperator. I'll update the comment.

# Test quoting.

'single' 'var: $var' 'subst: $(ls)' 'esc: \n \x01 \001'
"double" "var: $var" "subst: $(ls)" "esc: \n \x01 \001" # TODO: these last 2 are not highlighted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my machine, without this PR or other recent changes:

  • The $( and ) inside double quotes are highlighted, contrary to what the comment says.

  • The \n \x01 \000 are highlighted, contrary to what the comment says… but they shouldn't be:

    % print -r -- "\n \x01 \001" 
    \n \x01 \001

Comment on lines 12 to 13
'escape: \" \' \` \\ escape' # TODO: maybe highlight \' as an error?
$'escape: \" \' \` \\ escape'
Copy link
Contributor

@danielshahaf danielshahaf Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Careful here. Since \' is perfectly valid syntax, this line has two single-quoted strings: one with contents escape: \" \ and another with contents # TODO: maybe highlight \ [github is eating the leading whitespace]. There isn't any comment on this line at all, either.

Thus, if someone thinks that's a comment and adds the word "don't" to it, line 13 would no longer have a $'…' string in it. That seems somewhat brittle.

@arp242
Copy link
Contributor Author

arp242 commented Apr 29, 2022

Thanks for the comments @danielshahaf; I mostly just added a few tests for whatever came to mind as an initial PR to set up testing; I didn't really look/test too carefully at what it ought to do.

For now I tweaked the comments a bit; I'll look at the issues a bit more in the future.

@chrisbra
Copy link
Owner

okay, thanks. Appreciate the work on tests :)

@chrisbra chrisbra merged commit e1648e7 into chrisbra:master May 19, 2022
@arp242 arp242 deleted the test branch May 19, 2022 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants