Configuration-based PluginTestHarness instead of plugin object#1468
Configuration-based PluginTestHarness instead of plugin object#1468SimonSapin wants to merge 2 commits intomainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
garypen
left a comment
garypen
left a comment
There was a problem hiding this comment.
I've speed-read through the changes and I think the direction is good.
I have got one question: How will a user know the valid configuration to supply for a plugin?
Apart from that, I really like the clean-up effect on the examples.
It is true that when a user writes a |
SimonSapin
force-pushed
the
simon/harness
branch
2 times, most recently
from
6bce52e to
da8ba6c
Compare
SimonSapin
force-pushed
the
simon/harness
branch
from
bba31ef to
07831b0
Compare
o0Ignition0o
left a comment
o0Ignition0o
left a comment
There was a problem hiding this comment.
I like where this is going!
There's a few things I m not sure I understand, but it looks great overall!
| .build() | ||
| .await?; | ||
| let graphql = harness.call_canned().await?.next_response().await.unwrap(); | ||
| assert_eq!(graphql.errors, []); |
There was a problem hiding this comment.
nit
| assert_eq!(graphql.errors, []); | |
| assert!(graphql.errors.is_empty()); |
This imo would look more symetrical with the kinds of assertions apollo-rs does
| let config = CSRFConfig::default(); | ||
| let non_preflighted_request = RouterRequest::fake_builder().build().unwrap(); | ||
| let non_preflighted_request = RouterRequest::fake_builder() | ||
| .header("content-type", "text/plain") |
There was a problem hiding this comment.
does the header() method replace or append a header?
Raising this because the changelog mentions When no Content-Type header is specified, this builder will now default to application/json which makes the request be accepted by CSRF protection. so I'd suspect the header value to now be ["application/json", "text/plain"] which would be kinda surprising?
| let result = replace_layer(Box::new(telemetry)); | ||
| if cfg!(not(test)) { | ||
| result.expect("set_global_subscriber() was not called at startup, fatal"); | ||
| } |
There was a problem hiding this comment.
this should not be necessary once #1463 lands. And with the current version of the code it smells like a workaround for a deeper issue 🙃
so idk, either dig into it or wait for the fix 😁
| /// Returns `None` if the name or `Plugin` type does not match those given to [`register_plugin!`], | ||
| /// or if that plugin was not enabled in configuration. | ||
| pub fn plugin<Plugin: 'static>(&self, name: &str) -> Option<&Plugin> { | ||
| self.plugins.get(name)?.downcast_ref::<Plugin>() |
There was a problem hiding this comment.
Oh this is why we have the downcast magic?
I see!
|
After trying to follow this up by porting more things to use A lot of use cases involve manipulating a plugin instance. Already this PR does trait object downcasting to work around that, but having stuff in Then I’m considering generalizing the harness API: instead of a |
|
I will soon submit a PR with a different approach that supersedes this PR. |
5 participants
No description provided.