feat(cli): add test listing and allow users to parse a specific test number

[WillLillis]

I was interested in implementing some of the changes described in #2726. Namely,

Implement a stable ordering for all tests stored in the test folder by sorting test files and sorting all inner test by titles. I suppose tests already works in this way.

I used the existing ordering already in place from the test subcommand.

Add a --test-input <test_number> flag the parse command that would list all test tree with numbers and when called with a test number would use that test source text as an input for the parse command.

When provided with a test number x, parse --test-input x will parse the contents of test x. Specifically, the test's contents are extracted, written to a temporary file, and then parse continues execution with the path to said temporary file. Listing all of the tests with their numbers seemed redundant with test --list (described below), so I didn't implement that.

Also it's possible to extend the test command with a list sub command for more comfortable tests listing and picking and it may be able optionally to list test inputs after test titles and may be even expected trees.

Running test --list will list tests with their respective numbers. This should still respect the --filter, --include, and --exclude arguments if provided. It currently only prints each test's name and leaves the inputs/ expected trees out. I'm assuming there will be further discussion on what should be printed here?

Happy to make fixes, refactor, and/or split this up as needed. Hope this helps! :)

Closes #2726

[amaanq]

Hey this is a nice idea, and sorry for not reviewing it sooner (lot on the plate). I think this can be improved in a few ways.

1. I'd like it if the shorthand notation of test-input would be -n, so maybe renaming that to --test-number or --number would be better.
2. I think --list should run the tests with the test numbers printed out. Running tests does not take that long to begin with, and the current "display the test number without running" feels weird, so it might even make sense to just list the test numbers by default, it's not that intrusive imo (and makes it clear to the grammar author which test is number x/y/z always). Ideally you should align the test names, so the size of the test number doesn't affect the position of a name. Here's what I mean w/ your current --list flag:
   
   In test 100, the start of the test name, "Literal", is offset 1 character to the right compared to the test name above it since the size of the test number is 3, not 2. This does not look great but is an easy fix.

Overall the idea is good though, and I'm hoping you can improve this with the suggestions/feedback I've provided :)

[WillLillis]

sorry for not reviewing it sooner (lot on the plate)

No worries, y'all have been doing a ton of cool stuff with the project lately!

I'd like it if the shorthand notation of test-input would be -n, so maybe renaming that to --test-number or --number would be better.

👍

I think --list should run the tests with the test numbers printed out. Running tests does not take that long to begin with, and the current "display the test number without running" feels weird, so it might even make sense to just list the test numbers by default, it's not that intrusive imo (and makes it clear to the grammar author which test is number x/y/z always).

Makes sense to me 😄

Ideally you should align the test names, so the size of the test number doesn't affect the position of a name.

That seems fair. Since the tests are accessed in a recursive manner, it's hard to know the total number of tests up front. I think it probably makes sense to have a fixed width for the test number. I checked over some of the more popular grammars and it seems like 3 digits should be fine for this (unless there's grammars out there with 1000+ tests). Alternatively the code could do a quick dry run to iterate through and count all the tests, but I imagine that's a bit overkill.

Thanks so much for taking a look! I should be able to spend some time on this soon! :)

[WillLillis]

Here's a first pass on your feedback. I now have the test numbers printing out (with a set width of 3 characters) by default.

I ran into some issues maintaining a consistent numbering when the --filter, --include, and --exclude options were used. To solve this I moved the filtering logic in run_tests() from the TestEntry::Group match case to the TestEntry::Example case, as maintaining a consistent numbering with the group level filtering would have required some extra logic to traverse and count the skipped tests. The downside of this is that empty groups will still have their name printed out. For example, tree-sitter-python's tests run with --filter escape:
I'm happy to re-implement it the other way, but I figured going with the simpler approach first made more sense.
It was a tiny bit messier but maintaining consistent numbering while preventing empty group names from printing wasn't that bad.

Let me know if there's anything else I can do to improve this. :)

[amaanq]

mind rebasing? I can take a look & get it in then

[WillLillis]

On it :)

[amaanq]

what happened to --list? would it just be better to keep the numbers visible by default?

[amaanq]

I just pushed some changes, including squashing your commits into one, as well as

• cleaning up the temp file when done
• not calling to_owned to avoid an allocation, we only need to read from the data to write it to disk, so we can just return &'test [u8] in get_test_contents
• top level corpus is removed
• rebased on top of master
• prefer let else for early bails, it's just a lot nicer than a match where we don't map/perform some logic in the Ok case

[WillLillis]

what happened to --list? would it just be better to keep the numbers visible by default?

Yeah I had removed --list so that the numbers would just print by default. I'm happy to change that if you'd like it as a separate option :)

I just pushed some changes, including squashing your commits into one, as well as

• cleaning up the temp file when done
• not calling to_owned to avoid an allocation, we only need to read from the data to write it to disk, so we can just return &'test [u8] in get_test_contents
• top level corpus is removed
• rebased on top of master
• prefer let else for early bails, it's just a lot nicer than a match where we don't map/perform some logic in the Ok case

The let else for early bails is indeed much nicer, and I really like avoiding the allocation in get_test_contents. I'll keep these lessons in mind going forward, really appreciate it!

[amaanq]

tbh, it's fine as a default. it's probably good for authors who are in a rapid developing & testing cycle without having to remember to use the --list flag.

I do think we should use 1. instead of 1:, it's a little cleaner, and we should also start indexing at 1 since a "zeroth" test looks funky.

I'll keep these lessons in mind going forward, really appreciate it!

No problem 😁 I also think we should just use tempfile and pass in the given file since it'll auto delete on drop, which also takes care of the case when an error propagates upwards before the tests finish

[amaanq]

Ok, scratch that last idea. temp files are annoying since tempfile uses O_TMPFILE, which creates an unnamed file actually (interesting tidbit), and wrangling around this in the path iteration+parsing code is much more annoying than leaving one small file in the tmp folder in case it errors out earlier on

[WillLillis]

I do think we should use 1. instead of 1:, it's a little cleaner, and we should also start indexing at 1 since a "zeroth" test looks funky.

Sounds good!

I also think we should just use tempfile and pass in the given file since it'll auto delete on drop, which also takes care of the case when an error propagates upwards before the tests finish

Wasn't aware of this dependency, but that's really cool! I was trying to work out how to pass the file around with changing too much of the existing iteration, but like you said it was a little annoying. I was taking a look at the docs and TempPath looked promising. It's a path that deletes its corresponding file when drop is called, which seems to be exactly what we want here.

EDIT: Oh nevermind, I see what you mean. 😆

[amaanq]

Okay, coming back to this

I opted to print the test # and name - it was a little unclear which one was actually being parsed, and I also account for users starting their count at 1. I also used coloring for the test name itself, similar to the ts t output, but still respect NO_COLOR. I think this is good to go, we don't need to worry about deferring deletion of the temp file on drop - it'll be a few KB at most, and if the users are worried about running out of space because of this I think they have bigger problems (not to mention unix-like systems barring macos tend to automatically clean up old temp files anyways, with systemd being the most aggressive one iirc - files older than 10 days are fair game). We can revisit that later if it poses to be a problem, but I doubt it will

[amaanq]

also handles multi grammar repos with specific attributes now as well, that was nagging me