sql: move query result checking in testutils/sqlutils #10646
Conversation
|
Looks to me like this is increasing the number of magic values in these tests. Can that be avoided? Also, as always, it's a bummer that this will produce unpleasant error line numbers. Reviewed 1 of 3 files at r1. Comments from Reviewable |
|
The magic values (number of KVs) are coming from the expected strings just above and are very easily deduced just by looking at them. Honestly I feel it's more clear like this, the Review status: 1 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
OK, I'll take your word for it. Errors produced by this are still bad, but not worse than before, so LGTM (with the caveat that whatever future use you had in mind for this will also get bad error messages). Reviewed 2 of 3 files at r1. Comments from Reviewable |
|
Yeah, it's not the best, but it's an improvement since all messages now show the query. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
Actually, it should be easy to just get the caller once and then display it in the error message, I'll do that. Comments from Reviewable |
Moving code to verify the result of a Query to sqlutils, where it can be used by other tests. In the descriptor mutation test we now check the number of keys against known values rather than relying on checkQueryResponse to count the number of NULLs in the expected result (which we know). Embedding a SQLRunner in mutationTest which allows us to use the new function directly, as well as the other Fatal-on-error wrappers.
RaduBerinde
force-pushed
the
check-query-result-cleanup
branch
from
f67a7ac to
97415e1
Compare
|
Updated. |
|
Reviewed 3 of 3 files at r2. Comments from Reviewable |
all files reviewed at latest revision, all discussions resolved, all commit checks successful.
Moving code to verify the result of a Query to sqlutils, where it can be used by
other tests. In the descriptor mutation test we now check the number of keys
against known values rather than relying on checkQueryResponse to count the
number of NULLs in the expected result (which we know).
This change is