initial rework of syntest to be usable internally #185

keith-hall · 2018-07-02T12:36:04Z

this is an initial rework of the syntest example, to make it usable internally, as it would be useful for parser tests (see #180 (comment)). I think it will need some more work to properly support assertions that extend over the end of the line (#175) and spill over onto not only the next line, but also the line afterwards etc. although such a scenario may never come up in real life use.


        initial rework of syntest to be usable internally

trishume

Just some architectural requests to make it fit better as a public API.

Let me know if you made any real changes to the main body of the function you moved, I didn't read it, assuming it's just a copy-paste.

trishume · 2018-07-02T15:04:23Z

examples/syntest.rs

-use syntect::highlighting::ScopeSelectors;
-use syntect::easy::{ScopeRegionIterator};
+use syntect::parsing::{SyntaxSet};
+use syntect::easy::{SyntaxTestFileResult, SyntaxTestOutputOptions, process_syntax_test_assertions};


I don't think this should go in the easy module.

That's for simple wrappers for common use cases. Syntax tests are neither a wrapper around a more advanced API, nor a common use case.

They should probably go in their own syntax_tests module or something

trishume · 2018-07-02T15:04:23Z

src/easy.rs

+    Success(usize),
+}
+
+pub fn process_syntax_test_assertions(syntax: &SyntaxDefinition, text: &str, testtoken_start: &str, testtoken_end: Option<&str>, out_opts: &SyntaxTestOutputOptions) -> SyntaxTestFileResult {


If this is now going to be a public API, could you add a doc comment with short descriptions of what the parameters do? Also useful for our own reference.

trishume · 2018-07-02T15:04:23Z

src/easy.rs

@@ -174,6 +174,236 @@ impl<'a> Iterator for ScopeRegionIterator<'a> {
    }
 }

+#[derive(Clone, Copy)]
+pub struct SyntaxTestOutputOptions {


Now that this is an API, it seems a bit weird for it to just print things to stdout.

Maybe you could add a lifetime parameter to this struct and add a output: &'a io::Write member. Alternatively you could add an extra parameter for the output to the process function.

Then the function then uses writeln! instead of println! to write to, and the process function would probably need to return an io::Result. Then syntest and possibly also unit tests could just pass stdout.


        put syntax test functionality in it's own module

keith-hall · 2018-07-02T16:48:21Z

I have moved it to a new syntax_tests module and added a doc comment 👍
The main function has been modified; it used to read line by line, parsing as it went - now it reads all the syntax test assertion positions/details first, then parses line by line afterwards as a separate step and compares the results.
In terms of using io::Write, great idea. I think it's a little beyond my skillset and free time at the moment, so it'd be great if someone else wouldn't mind contributing this. It will be necessary to also modify Util::debug_print_ops - perhaps it'd make sense to rename it debug_write_ops, take an io::Write as a parameter, and create a new debug_print_ops which will just pass in stdout() (or https://doc.rust-lang.org/std/io/struct.Stdout.html#method.lock (I'm not sure which is better)).


        add more tests for finding syntax test assertions

robinst · 2018-07-03T08:22:50Z

Nice, thanks for picking this up :)!

In the current form, I don't think we should make it a public API. An API with the only output being some printed text doesn't seem very useful generally.

So I would do either one of these two things:

Change it to pub(crate)
Move the printing parts back to the syntest example, and change the API to return plain structs with the information about the assertions instead.

(With the latter, we could do some nice things such as formatting as HTML so you can more easily see the actual scopes, show the results in a text editor instead, etc.)

keith-hall · 2018-07-03T08:47:02Z

Printed text isn't the only output - the API returns the successful/failed assertion count too ;)

I would guess that changing the API to return structs with the information about the assertions, and printing them from the syntest example would mean that we would lose the "instant" nature of the results. i.e. currently, one would see in the output about failures at the top of the syntax test file without having to wait for the whole rest of the file to be evaluated. I'd be keen to keep this, if possible.
I like the idea of formatting the output, although I don't have any concrete ideas of how it could look at the moment. (Currently I just pipe the output of syntest into a file and review it in Sublime.) Maybe outputting to HTML (like synhtml), using a specified color scheme, and then showing the failures on top of that somehow?

robinst · 2018-07-03T09:13:20Z

Hehe :). Is it possible to make it return an Iterator with results? Then you could just iterate over that and print as the assertions come back.


        add tests to syntest to prove it works with assertions than span lines


        fix syntest seemingly skipping some files with crlf line endings

keith-hall · 2018-07-03T12:26:08Z

I think that returning an iterator with the results is a cool idea, it would allow a consumer of the API to fail fast when a failure occurs, if so desired. Though I guess it would likely prevent the use cases we mentioned above, whereby the (optionally colored) output could be combined with the syntax test results, as it would only iterate over those results and then the file would have to be parsed again to achieve an integrated view, which isn't ideal.
Also, it would mean all the consumers would have to track the number of failures and thus could cause code duplication when all they care about is the totals (for summary purposes).
But using io:Write instead wouldn't magically enable this either, so maybe it's not a realistic goal at the moment?

Marking the API for internal use only would solve needing to make further complicated changes at the moment. But changing the method to pub(crate) seems to prevent the syntest example from seeing it, and I get warnings about dead code. Any ideas? :)

trishume · 2018-07-04T01:05:24Z

Okay I'll try and give the new implementation a real review then, likely this weekend. If you'd like I can probably also do the io::Write stuff after I do that.

@robinst I agree it's not the greatest thing to have as a public API but:

there's unfortunately no pub(examples)
making it be a nice proper API may be a decent amount of work.
I've never been too stingy with public APIs in syntect and in this case if we implement a better API it should be pretty easy to reimplement it in terms of the new API, or do a major release and remove the old version (I doubt anyone will use it)


        add failfast mode to syntest, add more comments


        switch syntax tests back to a normal Vec instead of a VecDeque


        Merge branch 'master' into syntest


        make the get_syntax_test_assertions method public

this will allow other functionality to be developed that can use the same syntax test implementation and show the test results in a more friendly manner etc.

trishume · 2018-10-17T18:29:05Z

Sorry I'm so late on reviewing this, I wanted to use my limited syntect bandwidth for stuff that needed to be in the 3.0 release. This is still on my TODO list.

keith-hall · 2018-10-18T08:19:20Z

No worries, the work you did on the color schemes was awesome and indeed more important ;)


        add doc comment for get_syntax_test_assertions


        make syntax test header parsing public also


        remove regex dependency


        Merge branch 'master' into syntest


        handle new syntax test header format which caters for reindentation

initial rework of syntest to be usable internally

Loading status checks…

fc30e75

keith-hall added the enhancement label Jul 2, 2018

trishume requested changes Jul 2, 2018

View changes

put syntax test functionality in it's own module

Loading status checks…

fdaf8bd

add more tests for finding syntax test assertions

Loading status checks…

2dee5fc

Keith Hall added 2 commits Jul 3, 2018

add tests to syntest to prove it works with assertions than span lines

Loading status checks…

91af5e9

fix syntest seemingly skipping some files with crlf line endings

Loading status checks…

67c0e05

Keith Hall added 2 commits Jul 4, 2018

add failfast mode to syntest, add more comments

Loading status checks…

f485533

switch syntax tests back to a normal Vec instead of a VecDeque

Loading status checks…

6cba069

trishume mentioned this pull request Aug 2, 2018

Rename methods on SyntaxSetBuilder #195

Merged

keith-hall added 2 commits Oct 15, 2018

Merge branch 'master' into syntest

Loading status checks…

ab20070

make the get_syntax_test_assertions method public

Loading status checks…

1d754cf

this will allow other functionality to be developed that can use the same syntax test implementation and show the test results in a more friendly manner etc.

keith-hall force-pushed the forkeith:syntest branch from 9ed3756 to 1d754cf Oct 17, 2018

add doc comment for get_syntax_test_assertions

Loading status checks…

b39cd4e

keith-hall force-pushed the forkeith:syntest branch from c75729c to b39cd4e Oct 21, 2018

keith-hall added 2 commits Oct 21, 2018

make syntax test header parsing public also

Loading status checks…

6edd291

remove regex dependency

Loading status checks…

77f0de1

keith-hall mentioned this pull request Sep 3, 2019

Update sublimehq/Packages to master sharkdp/bat#644

Merged

keith-hall added 2 commits Apr 14, 2020

Merge branch 'master' into syntest

133e9dd

handle new syntax test header format which caters for reindentation

Loading status checks…

59ddb37

keith-hall mentioned this pull request Aug 3, 2020

Large scale syntax regression tests sharkdp/bat#1124

Open

trishume / syntect

initial rework of syntest to be usable internally #185

initial rework of syntest to be usable internally #185

keith-hall commented Jul 2, 2018

trishume left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

keith-hall commented Jul 2, 2018

robinst commented Jul 3, 2018

keith-hall commented Jul 3, 2018

robinst commented Jul 3, 2018

keith-hall commented Jul 3, 2018

trishume commented Jul 4, 2018

trishume commented Oct 17, 2018

keith-hall commented Oct 18, 2018

trishume / syntect

Join GitHub today

initial rework of syntest to be usable internally #185

initial rework of syntest to be usable internally #185

Conversation

keith-hall commented Jul 2, 2018

trishume left a comment

This comment has been minimized.

trishume Jul 2, 2018 Owner

This comment has been minimized.

trishume Jul 2, 2018 Owner

This comment has been minimized.

trishume Jul 2, 2018 Owner

keith-hall commented Jul 2, 2018

robinst commented Jul 3, 2018

keith-hall commented Jul 3, 2018

robinst commented Jul 3, 2018

keith-hall commented Jul 3, 2018

trishume commented Jul 4, 2018

trishume commented Oct 17, 2018

keith-hall commented Oct 18, 2018