Clean up test_parser.py, use xFail instead of skip where appropriate

[MegaIng]

This is something I did while working on the Lark.scan implementation. I wanted to use xfail and decided that it should be quite useful for many other tests as well. I also deleted a few old comments/completely skipped tests suggesting features that will probably never be implemented.

I adjusted a few of the conditions based on the actual behavior of the code, for example cyk does actually support the tree less transformer argument. It doesn't have quite as big of a benefit, but it does work.

[erezsh]

Why change the skip to xfail? xfail implies the test should be fixed in the future, i.e. failing for now but should work. (I'm sure you know pytest supports skip too)

Also, I'm wondering if this PR won't cause too many collisions with the lark.lark PR..

[MegaIng]

xfail implies the test should be fixed in the future, i.e. failing for now but should work

I somewhat disagree: While this is a possible meaning, I would in this case interpret it as these should fail, and if they don't the tests aren't actually testing for what they should, without any promises about the future. E.g. a test case that is designed to specifically need the dynamic lexer should not function without it. But if you are strongly against this interpretation, that's fine by me.

Also, I'm wondering if this PR won't cause too many collisions with the lark.lark PR..

I don't think so? That PR hasn't touched test_parser.py at all.

[erezsh]

Oh right, sorry my bad.

As for xfail, I don't think it means that the test won't function, but rather that the test is correct but the source code isn't. So for example xfailing ambiguity on lalr would mean that we expect lalr to support ambiguity, and it's temporarily broken.

If it's not a bother, could you please change xfail into skip?

[MegaIng]

As for xfail, I don't think it means that the test won't function, but rather that the test is correct but the source code isn't. So for example xfailing ambiguity on lalr would mean that we expect lalr to support ambiguity, and it's temporarily broken.

This is a possible interpretation, and probably the more common one. But I don't think the feature itself is all too prescriptive in this regard.

As a concrete example, consider the tests that use the tree less transform parameter. These only work for lalr (and cyk) and should produce an error message for earley, meaning that the tests using this should not pass, i.e. they are expected to fail. We currently have no plans to ever implement this feature (AFAIK), but it's still an ok usecase of xfail IMO. If you think this is to confusing, I will revert that part of this PR.

[erezsh]

Yes, using Earley in that case should produce an error, but usually that would be caught in an AssertRaises and not with the xfail mechanism.

But actually, if you feel strongly about it that's fine by me. I usually just use xfail as a loose TODO marking, so not much loss there.

Edit: I take it back. I just ran it in the console and got a huge wall of xfail warnings.. Let's just use skip, okay?

[MegaIng]

Edit: I take it back. I just ran it in the console and got a huge wall of xfail warnings.. Let's just use skip, okay?

Sure, did that. Now it's a huge wall of skip warnings instead.

[erezsh]

Thanks!

For whatever reason, the wall of skips is 3 times smaller than the wall of xfails was.

I also think to make it even less, by replacing a few of the skips with a simple if stmt. But I can do that later, after this has been merged.