Framework for testing interactions with CloudClient
#127
Conversation
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
src/commands/link.rs
Outdated
| Ok(_) => panic!("Expected error"), | ||
| Err(err) => assert_eq!( | ||
| err.to_string(), | ||
| r#"Database "does-not-exist" does not exist"# |
There was a problem hiding this comment.
I'm a little worried about doing exact matches on error messages as this can be pretty brittle, but I think it's fine to leave for now and see how it works out in practice.
…all counts Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
kate-goldenring
force-pushed
the
test-structuring
branch
from
45303bf
to
4e5fa59
Compare
There was a problem hiding this comment.
This is great to see. I left a few comments but nothing showstopping.
I did, as expected, find the mock-API style of testing a bit tricky. The lack of asserts at the end of many of the tests threw me - but of course the assertion is actually in setting up the expectation! I think this is part of why I prefer fakes, so that I can make explicit assertions about before and after state rather than setting expectations about specific actions. However setting up fakes for something like this would be a much greater amount of work, and this seems like a pragmatic way to get some unit testing going. Thanks for driving it!
| cloud-openapi = { workspace = true } | ||
| mime_guess = { version = "2.0" } | ||
| mockall = "0.11.4" |
There was a problem hiding this comment.
The other reference to this is a dev dependency. Does the cloud crate need it at runtime?
There was a problem hiding this comment.
It does because it is in a separate crate. I put it behind a feature flag now
There was a problem hiding this comment.
Hmm... you should be able to put this behind a dev-dependency instead of a feature. Are you running into issues with that?
There was a problem hiding this comment.
Yeah. I cannot seem to figure out how to put it behind a dev dependency and then use it in a testing scope from another crate that imports the crate. Can you import types that are behind a dev dependency from another crate?
There was a problem hiding this comment.
Seems i am running into rust-lang/cargo#8379
There was a problem hiding this comment.
@rylev I am merging this but happy to do a follow up pr if you have ideas about how to put it behind a dev dep
| @@ -1 +1,4 @@ | |||
| pub mod client; | |||
| mod client_interface; | |||
| pub use client_interface::CloudClientInterface; | |||
| pub use client_interface::MockCloudClientInterface; | |||
There was a problem hiding this comment.
I guess that answers that question! #[cfg(test)] maybe?
There was a problem hiding this comment.
looks like code compiled with cfg(test) is not accessible to external crates, so I wasn't able to reference the mock type after putting it behind this test flag. But I was able to put it behind a feature flag and ill push up a change for that
src/commands/link.rs
Outdated
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_sqlite_link_success() -> Result<()> { |
There was a problem hiding this comment.
To me, what this test tests is not "success" but something more like "if the database exists, the link is created". It would be great to express this more explicitly. I'm not sure if it can be done in the test name - it might be too long! - but if not then maybe a comment?
Similarly with many of the other tests e.g. "if the label already points to the requested database, it errors" or "if the label already points to a different database, it..."
There was a problem hiding this comment.
I made the sqlite test names more explicit and readable. Let me know what you think.
There was a problem hiding this comment.
Thanks! The link test names are definitely more expressive!
kate-goldenring
force-pushed
the
test-structuring
branch
from
65bd9fa
to
3ffbb64
Compare
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
kate-goldenring
force-pushed
the
test-structuring
branch
from
3ffbb64
to
0ae71e9
Compare
This is a proposed approach to increasing our unit tests in this repository by making the
CloudClientmockable. It uses themockallcrate by having theCloudClientimplement aCloudClientInterfacethat can be mocked. Also has a few tests for examples.Pros to this approach:
Cons:
(I'm sure there are other pros and cons i am forgetting)
fixes #124