Open rnewman opened 7 years ago
Can you give a pointer to the tests in question, and something that looks nice? The tests presumably are here https://github.com/mozilla/mentat/blob/5af7082165061f530dd77f14a13196230cce5c34/query-algebrizer/src/cc.rs#L601
The tests are indeed those; they look like:
cc.apply_pattern(&schema, &Pattern {
source: None,
entity: PatternNonValuePlace::Variable(Variable(PlainSymbol::new("?x"))),
attribute: PatternNonValuePlace::Ident(NamespacedKeyword::new("foo", "bar")),
value: PatternValuePlace::Constant(NonIntegerConstant::Boolean(true)),
tx: PatternNonValuePlace::Placeholder,
});
You can imagine a version that runs through the pattern
rule of the query parser after an EDN parse, allowing us to write:
let pattern = Pattern::from_edn("[?x :foo/bar true _]");
cc.apply_pattern(&schema, &pattern);
which is substantially clearer.
@rnewman also filed #317 to make debug output of ConjoiningClauses
nicer; I'd like to gently suggest that we turn clauses into EDN and use the EDN matcher @victorporof is working on in #271 to make sure we're getting the desired output. It might be that the EDN matcher would need to grow some string/regex features to support these tests, but that's oaky.
This would be a nice unification of our testing patterns, and might again allow to run the same tests against the Clojure and Rust implementations, a la #188. (For the record, I think this is both less likely and less valuable than sharing the transactor tests between the Clojure and Rust implementations.)
One of the problems with using textual/structural comparison is that it forces tests to be written in terms of exact comparison, rather than expressing expected arbitrary invariants. It also makes it less natural to test intermediate states.
The majority of the testing I do would not use EDN comparison, because there's always something for which I want to say "there's an entry for ?x
but I don't care what it is", or "the thing in this map matches this thing in this array over here", or whatever.
Furthermore, making test cases test some output format breaks one very good piece of unit testing advice, which is that tests should be orthogonal. Each thing you alter in your code should, ideally, break one test. Template-based testing means that you break most of your tests, because each one redundantly checks the same things, obscuring the real breakage. Add a field to the output? Now you need to put a correct value for that field in every test result string.
I plan to use result-format testing for queries themselves — imagine a kind of "EDN SQL" — but I don't think it's particularly useful for pattern- or CC-level tests.
@rnewman this is clearly case-by-case. I think the things I've sketched for the EDN matcher hit what you would actually use, though:
?symbol
, and make sure that expected use agree easily_
...
, focusing your test on the small piece you care about.I leave this to your discretion.
They currently painstakingly construct the input pattern and schema. It'd be nice if they were easier to read.