[test] Check that CockroachDB tables align with Diesel tables#9927
[test] Check that CockroachDB tables align with Diesel tables#9927
Conversation
smklein
force-pushed
the
crdb-alignment
branch
from
e0a8612 to
7055677
Compare
| pub columns: fn() -> Vec<(&'static str, &'static str)>, | ||
| } | ||
|
|
||
| #[distributed_slice] |
There was a problem hiding this comment.
This is sorta the implementation-specific artifact to come out of the table! macro in nexus/db-schema/src/schema.rs, but basically:
- I made a new
table!macro - looks and smells like the old table macro
- it also auto-registers the table with this big static list
This means that "just by declaring your table macro, we know your table exists", and you don't need to type the table name again in some big list somewhere else.
For folks updating schema.rs: it means there's no observable changes.
For our tests: We now know the list of all schema-declared tables, as well as the types of all columns within.
Aside: linkme is kinda dope
There was a problem hiding this comment.
Thinking out loud - could/should the call to __register_table!() and DIESEL_TABLES itself be gated behind #[cfg(test)]? I'm honestly not sure, if nothing in production uses it anyway, but it might be nice to keep a library self-described as "safe cross-platform linker shenanigans" in just tests. 😅
There was a problem hiding this comment.
This required me to convert the integration test to a unit test for nexus-db-schema, and reshuffle some dependencies, but done in 7d8d1fe
| // Check for Array<...> | ||
| let kind = if let Some(array_inner) = inner |
There was a problem hiding this comment.
Cockroach doesn't support nested arrays, fwiw, so this is as far as we'd go
|
As an FYI, my next step is going to be: "reduce the size of the exception-list EXPECTORATE files". But this PR really is the back-stop regression test which should prevent this problem from getting worse. |
smklein
changed the title
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
Thanks, this is an excellent guard to put in place. Just a handful of small questions/nits.
| pub columns: fn() -> Vec<(&'static str, &'static str)>, | ||
| } | ||
|
|
||
| #[distributed_slice] |
There was a problem hiding this comment.
Thinking out loud - could/should the call to __register_table!() and DIESEL_TABLES itself be gated behind #[cfg(test)]? I'm honestly not sure, if nothing in production uses it anyway, but it might be nice to keep a library self-described as "safe cross-platform linker shenanigans" in just tests. 😅
| impl std::fmt::Display for ScalarType { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| ScalarType::BigInt => f.write_str("diesel::sql_types::BigInt"), |
There was a problem hiding this comment.
Turbo-nit, feel free to ignore: this could be shorter with just one f.write_str():
let s = match self {
ScalarType::BitInt => "diesel::sql_types::BitInt",
// ... all the variants ...
ScalarType::Enum(name) => name.as_str(),
};
f.write_str(s)| let mut errors: Vec<String> = Vec::new(); | ||
| let mut nullable_exceptions: Vec<String> = Vec::new(); | ||
| let mut column_order_drift: Vec<String> = Vec::new(); | ||
| let mut known_drift: Vec<String> = Vec::new(); |
There was a problem hiding this comment.
Naming nit - I assumed from this variable name that we'd be populating it with types we know have drifted, but IIUC this is populated with types we discover have drifted. Could we rename this to make that clear? mismatched_columns or something like that. Keeping the file we compare against as known_drift.txt makes sense, though, since those are the ones we do know about.
There was a problem hiding this comment.
Sure thing, renamed to mismatched_columns in 7d8d1fe
|
|
||
| // Sort and compare each anomaly category against its expectorate file. | ||
| nullable_exceptions.sort(); | ||
| expectorate::assert_contents( |
There was a problem hiding this comment.
Should we treat these files more like the auth tests in Nexus, and intentionally avoid using expectorate so they're not easy to overwrite? I assume once the work to fix all the known discrepancies, we should never overwrite the empty files again (and we don't want to make it look like it's a thing someone should do, which it might if the test says "run again with EXPECTORATE=overwrite").
There was a problem hiding this comment.
Sounds good, I was doing EXPECTORATE for convenience, but agreed that I only want this to shrink.
It'll require manual editing now, but that's probably fine
2 participants
Part of #9925