tree-sitter-grammars: Enable grammar testing #391520
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
6.topic: emacs
nix-owners
bot
requested review from
matthewbauer,
AndersonTorres,
ttuegel,
jian-lin and
adisbladis
jian-lin
requested review from
ambroisie,
uncenter,
amadaluzia and
khaneliman
and removed request for
jian-lin
ambroisie
reviewed
Mar 20, 2025
There was a problem hiding this comment.
I'm surprised the emacs plug-in doesn't seem to do the same as nvim-treesitter (separately locked grammars, specific to the plug-in).
Should we worry about reporting a grammar as broken "just" because some its tests aren't passing, if it builds and actually parses its inputs? I'm kind of surprised at the number of grammars which turned up broken = true here.
| @@ -76,6 +76,7 @@ let | |||
| src = grammar.src or (fetchGrammar grammar); | |||
| location = grammar.location or null; | |||
| generate = grammar.generate or false; | |||
| isBroken = grammar ? isBroken && grammar.isBroken; | |||
There was a problem hiding this comment.
Suggested change
| isBroken = grammar ? isBroken && grammar.isBroken; | |
| isBroken = grammar ? isBroken or false; |
There was a problem hiding this comment.
isBroken is always false indeed.
isBroken = grammar ? isBroken;There was a problem hiding this comment.
Your suggestion raises an error. I am not sure why...
nix-repl> attr = {a=true; b=false;}
nix-repl> (attr ? c) or false
error: undefined variable 'or'
at «string»:1:1:
1| (attr ? c) or false
| ^
nix-repl> attr.a or false
trueMaybe instead: broken = grammar.broken or false.
| @@ -58,4 +65,9 @@ stdenv.mkDerivation ({ | |||
| fi | |||
| runHook postInstall | |||
| ''; | |||
|
|
|||
| meta = { | |||
| broken = isBroken; | |||
There was a problem hiding this comment.
Might as well rename it to broken and inherit it here IMO.
There was a problem hiding this comment.
isBroken is always false (since it is generated by the update script), the name indicates that is flagged as broken.
I don't mind changing it to broken.
The reason behind this choice is to avoid updating all the JSON files without a new and unnecessary attribute.
Comment on lines
-96
to
-97
|
|
||
|
|
There was a problem hiding this comment.
Belongs in a different commit.
|
|
||
| if not check_grammar(data): | ||
| # Test the grammar with Tree-sitter and set the isBroken flag accordingly | ||
| data['isBroken'] = not check_grammar(data) |
There was a problem hiding this comment.
Related to my earlier comment about isBroken/broken naming.
...cations/editors/emacs/elisp-packages/manual-packages/tree-sitter-langs/default-grammars.json
Show resolved
Hide resolved
...plications/editors/emacs/elisp-packages/manual-packages/tree-sitter-langs/update-defaults.py
Outdated
Show resolved
Hide resolved
...plications/editors/emacs/elisp-packages/manual-packages/tree-sitter-langs/update-defaults.py
Show resolved
Hide resolved
|
Honestly, I can't say. That's why it is important to have feedback. What I can say (working on this PR for almost a year now), is that the recent bump from tree-sitter 0.24 to 0.25 has increased the number of broken grammar. The lack of testing is probably at cause here. |
Very much appreciate the effort to add some testing. |
khaneliman
reviewed
Mar 20, 2025
amadaluzia
reviewed
Mar 20, 2025
Comment on lines
153
to
155
| if not check_grammar(data): | ||
| # Test the grammar with Tree-sitter and set the isBroken flag accordingly | ||
| data['broken'] = not check_grammar(data) |
There was a problem hiding this comment.
I think this could just use a variable instead of having to run the check twice.
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
2 times, most recently
from
a81917f to
1439f9f
Compare
|
We should keep the commit history clean- can we squash the fixups? |
I split everything into atomics commit for the review. Indeed, the goal is to merge, but should I do it know? Please be mindful of my time, git manipulations like that take time and can be error-prone. |
Thank you for the link; it was very informative. Sorry, I meant "now" (not "know") in my original message. I was asked to split everything into atomic commits for the review and then rebase once we agree on all the changes (discussion). At least, that's how I understood it. I will rebase, but I want to avoid unnecessary git manipulations unless a commit needs to be split because it doesn't meet the first commit convention (each commit must be a logical unit, though logical unit is likely subject to interpretation). |
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
from
1439f9f to
0cf6ba1
Compare
It is done! Please let me know if it works for you. |
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
2 times, most recently
from
2de8950 to
0044ec1
Compare
|
Appreciate organizing the commits more, makes it easier to step through changes. |
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
from
0044ec1 to
23d6584
Compare
Hi, I just:
Please let me know if you see something else. |
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
2 times, most recently
from
a58f0db to
e508012
Compare
|
One wrench to throw in here, I don't know if some of the changes need to target staging.. I think we've had |
I think that you are right, at least it was the case for this recent update of tree-sitter (#389796). If I understand correctly, to achieve that, I need to update the target branch in GitHub and rebase on top of staging? |
|
@adfaure you need to follow the contributing guide about how to retarget an MR, to avoid mass-pinging people. |
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
from
e508012 to
afdbc8a
Compare
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
from
afdbc8a to
868a440
Compare
Ok It's done, I have followed the guide. I hope it didn't trigger too many pings. Sorry, if it did. |
ambroisie
reviewed
Mar 22, 2025
| checkPhase = '' | ||
| runHook preCheck | ||
|
|
||
| # HOME is needed because tree-sitter needs a writable home folder |
There was a problem hiding this comment.
Comment should go with writableTmpDirAsHomeHook.
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
2 times, most recently
from
97ca3ae to
0ee981b
Compare
adfaure
force-pushed
the
tree-sitter-grammars-activate-tests
branch
from
0ee981b to
50e28d7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Context
While working on #320783 with @fricklerhandwerk. We used
tree-sitter-grammarsto build Python bindings, we explored ways to test these grammars. We found that the best approach is to use Tree-sitter's dedicated testing framework (documentation).This PR introduces a check phase using
tree-sitter test.Changes
Activate Grammar Testing
Adds a test phase and an
isBrokenattribute.Runs the tests during the update phase and sets the
isBrokenattribute if the tests fail.Update the Emacs Script
Emacs has an automated mechanism for updating grammars. This script is updated to use the
isBrokenflag.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.