search

lark-parser / lark

Lark is a parsing toolkit for Python, built with a focus on ergonomics, performance and modularity.
MIT License
4.8k stars 409 forks source link

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

Closed MegaIng closed 3 months ago

MegaIng commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.