Morgan Smith <[email protected]> writes: > Regardless, you should probably remove or ":expected-result :failed" > that one test case.
I did not write that test to say "this is the right behavior". I wrote it because the behavior surprised me when I first ran into it, and writing the test was how I made sure I actually understood what the code does -- not what I assumed it does. That is how I usually work my way into an unfamiliar corner of a codebase: a small assertion that says "yes, really, this is what happens here". Once written, that kind of test is also useful for the next person. If someone else later wonders "wait, does inserting a COLUMNS keyword really put it *there*?", the test answers the question immediately, with a runnable example, instead of forcing them to reproduce the surprise from scratch. This is what Michael Feathers calls a characterization test: https://michaelfeathers.silvrback.com/characterization-testing "We need to know when we are changing existing behavior regardless of whether we think it's right or not." The test documents behavior; it does not endorse it. I do see the tension with what Ihor wrote in that thread, and with the paragraph he added to testing/README in e1ef98202: tests in Org are sometimes the only reference for how the code *should* behave, so a passing assertion of awkward behavior can be misread as "this oddity is intentional". That is a fair worry. Where I would push back gently is that the worry is about how the test *reads*, not about whether the knowledge it captures is worth having -- and reading can be fixed with a comment or docstring ("this pins current behavior that surprised the author; see <thread>; the desired behavior is X"), in a way that ":expected-result :failed" cannot, because :failed silently drops out of a green run and gives the next reader nothing to grab onto. So my preference, if it is acceptable, is to keep the test and add a docstring/FIXME on it that makes its status explicit: this is a landmark for a known-surprising behavior, not an endorsement of it. That way the knowledge stays in the test suite, where it is runnable and discoverable, and the "this is intentional" misread is headed off in writing. Ihor, given that you wrote the README paragraph, I would value your read: 1. Is a clearly-labelled "landmark" test of this kind -- plain assertion + docstring saying "pins surprising current behavior, not a spec" -- something you can live with in the tree, or does it still cross the line the README paragraph is trying to draw? 2. If it does cross the line, would you prefer I rewrite the test to assert the *desired* behavior with :expected-result :failed, and move the "this is what actually happens today" note into a comment on the test? I can do that, I just want to be sure I am not throwing away the signal by accident. > Thank you for adding tests! I do love tests! Likewise -- and thanks for pushing on this, it is exactly the kind of review that keeps the test suite honest. Best, -- Slawomir Grochowski
