gvanrossum / patma

Pattern Matching
1.02k stars 64 forks source link

Using pattern matching in unit tests is hard #122

Open dmoisset opened 4 years ago

dmoisset commented 4 years ago

This may look as a rehash of #120, but it's not. I don't want to push for a specific solution, but I'd like to turn it into a question/concern to see if at least there's an agreement that there's a problem.

To summarize: I found testing a compelling scenario for pattern matching; after calling your tested code, you match the result to see if it is correct. I easily found several relevant examples with a very lightweight survey.

The current PEP allows me to simplify something like this

    assert not current.daemon
    assert isinstance(authkey, bytes)
    assert len(authkey) > 0
    assert current.ident == os.getpid()
    assert current.exitcode is None

into this:

    pid = os.getpid()
    match current:
        case Process(daemon=False, authkey=bytes(authkey), ident=.pid, exitcode=None) if authkey: pass
        case _: assert False, "mismatch"

Having a repeated pass block followed by a case _: assert False in every test is repetitive and noisy IMO. Is it possible to do better? Are we happy with this solution for now? Could we do something to improve this use case?

gvanrossum commented 4 years ago

Honestly, it feels wrong to write this using match/case. It "buries the lede", which is that we're asserting some property: Instead, writing it this way looks like we're asking an innocent question but then if the answer isn't right we shoot the messenger.

So maybe that just means I agree that there's a problem -- but I think the problem is that match is the wrong tool for this job.

dmoisset commented 4 years ago

Written this way it is a bit buried and that's sort of my point. There is a property I want to assert, and the property is "obj x matches pattern p". So it is a pattern matching problem, and the sequence of asserts (which must be in the right order) looks very similar to the sequences of ifs that we're saying we don't like (like the is_tuple example in the PEP.

Taking our "official example", the tests rely on the possibility of converting to a string and matching to full equality. If I wanted more fuzzy matching, or if my expressions weren't reliably serializable, how would you express "the result is the sum of two products"?

viridia commented 4 years ago

Another problem with this kind of assert is that it doesn't tell you what went wrong. Simply reporting 'mismatch' is not very helpful in diagnosing what failed.

One of the main advantages of a powerful, modern expectation library is that it won't just return a boolean "it worked" vs. "it didn't work" signal, but it will actually do a 'diff' of the expected value vs. actual value and print that out on the console.

But in order for that to work, your 'expected' value needs to be introspectable - it needs to be data-driven, and not just a bunch of code. That doesn't mean you are limited to just equality testing either - your 'expected' value can be a complex object containing both literals to be compared using equality, and matcher objects that have can have arbitrarily rich predicates.

The tradeoff, of course, is performance - walking a complex expectation value is going to be a lot slower than executing branches in VM bytecode - but for unit tests, we don't generally care about how fast assert is going to execute.

gvanrossum commented 4 years ago

Actually, pytest shows you exactly what went wrong. Try it.

dmoisset commented 4 years ago

Exactly, I was thinking in something what something like pytest could do with this and how helpful would be. But again the way I had to write it ended up placing the assert in a location where pytest will never be able to help me (they might if they recognize this specific structure, but I don't think I'd do that as a pytest dev).

A few things that could help me here are:

But pushing for any change is a bit pointless if everybody else is happy with that chain of asserts (i.e. the status quo) as the way to write this kind of tests.

viridia commented 4 years ago

I would argue against using match like this in unit tests for the same reason I would argue against code like the following:

if not isinstance(authkey, bytes):
  assert False

match, like if is a flow-of control construct, and is designed towards performance. But in a unit test you are less concerned about performance than you are about maintaining an auditable chain of causality, which built-in statements don't provide. In the above example, a framework like pytest cannot feasibly introspect the byte code for the test and identify what condition caused the failure, and the same would be true for use of match. This is why the best unit test frameworks (in many languages, not just Python) supply an entirely different set of conditional and flow-control constructs that supplant built-in statements. Best practices for unit tests recommend that tests be mostly linear in structure, without any fancy loops or conditionals, and should be readable as a document listing the author's assumptions about the code under test.

As Guido said earlier, it's the wrong tool for the job. I would beware of superficial similarities between the match statement and the desire to deeply validate attributes of an object in test - IMHO it's an example of an anti-pattern I call "over-factoring", in which two pieces of code are superficially similar in behavior and shape, but an attempt to generalize them reveals that there are enough subtle differences that the generic implementation turns into a mess.

dmoisset commented 4 years ago

I would argue against using match like this in unit tests

If by "like this" you refer to my concrete example, we 100% agree. That's why I put it like an example of "this is ugly can we do better"

match, like if is a flow-of control construct, and is designed towards performance

We have different visions... for me match is not about performance but all about readability when recognizing the shape of data and destructuring it (I really couldn't care less if it takes the same time or even a bit more than the corresponding complicated if statements).

a framework like pytest cannot feasibly introspect the byte code for the test and identify what condition caused the failure

It does pretty good with an if statement even if complicated. It could do it with match (probably without adding a lot of logic) if there was some way to tie together the pattern match to the assertion.

But going to the core question of the issue, after getting two "this is the wrong tool for the job". What is the right tool?

  1. Status Quo
  2. Pattern matching
  3. Other (which one?)

I'm proposing 2 (with something likely to be close but not exactly like the current proposal), what is your answer?

gvanrossum commented 4 years ago

Maybe we should just label this son-of-pepzilla and move on.

dmoisset commented 4 years ago

Maybe we should just label this son-of-pepzilla and move on.

Possibly. I was planning to include an "Applications of pattern matching" section in #113, and liked the idea of adding "testing" in that bucket, but it's not a requirement.