chore: add memory catalog test to handle table removal before schema deregistration #17307
Conversation
…e-removal-before-schema-deregistration-
|
@alamb Can you help me take a look if you have time? I just added a test. If it doesn't work, I can close it. Thanks |
|
|
||
| cat.register_schema("foo", schema.clone()).unwrap(); | ||
| schema.deregister_table("t").unwrap(); | ||
| assert!(cat.deregister_schema("foo", false).unwrap().is_some()); |
There was a problem hiding this comment.
I see this just follows the pattern of the existing tests in this module, so that is good
I think a better pattern for checking errors is to verify that the error is actually the error that is expected (to make sure it isn't an error in the test setup, for example).
Something like
| assert!(cat.deregister_schema("foo", false).unwrap().is_some()); | |
| let err = cat.deregister_schema("foo", false).unwrap_err().to_string(); | |
| assert!(err.contains("expected error message"), "Can't find expected in {err}")); |
That would be a nice follow on PR
There was a problem hiding this comment.
@alamb Thanks for the suggestion, but the above code will report an error
calledResult::unwrap_err()on anOkvalue: Some(MemorySchemaProvider { tables: {} })
There was a problem hiding this comment.
And referring to other tests, they are all written in a similar way, assert!(cat.deregister_schema("foo", false).unwrap().is_some()); Maybe it would be better to keep the original writing.
…e-removal-before-schema-deregistration-
Which issue does this PR close?
Contrast with the memory_catalog_dereg_nonempty_schema test
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?