Dev/jet/mapd 645/rulecrud spec #650
Conversation
| 2) simplified handlers for assessing operator calls (via sinon api) | ||
| 3) simplified assessment of flow through '.then' query callback evaluators, no matter | ||
| how many are chained, and easily test input/output of callbacks | ||
|
|
There was a problem hiding this comment.
hope you guys don't mind my usage of class docstring-style documentation on this one, per polarmobile/coffeescript-style-guide#39
|
@nmccready @jessecstewart @zacronos @tallented Looks like it's relatively easy for inadvertent data changes from sql inside specs. I know usually we would just test .toString() on the queries, but it would be nice to have a more solid safeguard just in case. Made a class to help facilitate that, and allow me to improve coverage on the service calls since they do some add'l processing, contain nested calls, etc; things that set it apart from regular crud class operators. In a nut shell, it just facades in sinon api among the chainable knex operators, nothin really too fancy, but super helpful and makes detailed testing on queries, callbacks, etc really easy. |
| expectedQuery = """insert into \"config_data_normalization\" (\"config\", \"data_source_id\", \"data_source_type\", \"data_type\", \"input\", \"list\",""" + | ||
| """ \"ordering\", \"output\", \"required\") values ('{\\\"DataType\\\":\\\"Int\\\",\\\"nullZero\\\":true}', 'CoreLogic', 'county', 'tax', '\\\"\\\"',""" + | ||
| """ 'general', '0', 'Int Param', 'false'), ('{\\\"DataType\\\":\\\"Int\\\",\\\"nullZero\\\":true}', 'CoreLogic', 'county', 'tax', '\\\"\\\"', 'general',""" + | ||
| """ '1', 'Another Int Param', 'false')""" |
There was a problem hiding this comment.
The double-quotes in the string don't need to be escaped when using """ as the outer delimiter. (It would be much more readable without the extra escapes.)
As a side note since you brought it up Justin, one consequence of code like this (testing the generated query string) is that our test is fragile -- it relies on how knex handles things internally. If knex makes a change and the order of the columns changes, for example, both knex and our code would still be doing things correctly, but this test would fail. That doesn't mean we have to stop doing it, but it does mean that ideally we could find a better way to handle things that is less fragile.
Also, knex currently has a bug causing it to put extra escapes in when generating a query string (but not when executing the query for real). So this test will break if knex fixes that bug. knex/knex#1021
There was a problem hiding this comment.
Ok let me try to revisit that then... I had a heck of a time with that for a while last night though, it wanted more escape in the values section.
I agree about the query string testing... classic nondeterministic element ordering and compiling. I think until now it's just been the lowest hanging fruit for us, so it's currently used often, and I'm just being consistent. Hopefully, some of the offerings of that SqlMock class might render those unnecessary (or just more manually spying).
There was a problem hiding this comment.
@zacronos yea, not getting that quoting to work any other way. ill catch up w/ you in chat.
|
LGTM, though I'd like to see the extra escapes removed for readability |
|
cool ok, so we got to the bottom of the string dilemma... i was adding extra slashes, and only needed one set of double-slash. Anyways, moot point now as I've removed those longer strings in lieu of the fact that those tests already test if |
|
LGTM |
…spec Dev/jet/mapd 645/rulecrud spec
| @@ -0,0 +1,148 @@ | |||
| _ = require 'lodash' | |||
| tables = require '../../backend/config/tables' | |||
|
|
|||
No description provided.